Re: [PATCH v3] dmaengine: qcom: Add ADM driver
Hi Jonathan,
On Sun, 2020-09-20 at 19:12 +0100, Jonathan McDowell wrote:
> Add the DMA engine driver for the QCOM Application Data Mover (ADM) DMA
> controller found in the MSM8x60 and IPQ/APQ8064 platforms.
>
> The ADM supports both memory to memory transactions and memory
> to/from peripheral device transactions. The controller also provides
> flow control capabilities for transactions to/from peripheral devices.
>
> The initial release of this driver supports slave transfers to/from
> peripherals and also incorporates CRCI (client rate control interface)
> flow control.
>
> Signed-off-by: Andy Gross
> Signed-off-by: Thomas Pedersen
> Signed-off-by: Jonathan McDowell
[...]
> +static int adm_dma_probe(struct platform_device *pdev)
> {
[...]
> + adev->core_clk = devm_clk_get(adev->dev, "core");
> + if (IS_ERR(adev->core_clk))
> + return PTR_ERR(adev->core_clk);
> +
> + ret = clk_prepare_enable(adev->core_clk);
> + if (ret) {
> + dev_err(adev->dev, "failed to prepare/enable core clock\n");
> + return ret;
> + }
It is better to only start enabling the hardware after all resources
have been acquired, see below.
> + adev->iface_clk = devm_clk_get(adev->dev, "iface");
> + if (IS_ERR(adev->iface_clk)) {
> + ret = PTR_ERR(adev->iface_clk);
> + goto err_disable_core_clk;
> + }
Move this up before the core_clk enable, and you can directly return the
error.
> +
> + ret = clk_prepare_enable(adev->iface_clk);
> + if (ret) {
> + dev_err(adev->dev, "failed to prepare/enable iface clock\n");
> + goto err_disable_core_clk;
> + }
> +
> + adev->clk_reset = devm_reset_control_get(>dev, "clk");
> + if (IS_ERR(adev->clk_reset)) {
> + dev_err(adev->dev, "failed to get ADM0 reset\n");
> + ret = PTR_ERR(adev->clk_reset);
> + goto err_disable_clks;
> + }
> +
> + adev->c0_reset = devm_reset_control_get(>dev, "c0");
> + if (IS_ERR(adev->c0_reset)) {
> + dev_err(adev->dev, "failed to get ADM0 C0 reset\n");
> + ret = PTR_ERR(adev->c0_reset);
> + goto err_disable_clks;
> + }
> +
> + adev->c1_reset = devm_reset_control_get(>dev, "c1");
> + if (IS_ERR(adev->c1_reset)) {
> + dev_err(adev->dev, "failed to get ADM0 C1 reset\n");
> + ret = PTR_ERR(adev->c1_reset);
> + goto err_disable_clks;
> + }
> +
> + adev->c2_reset = devm_reset_control_get(>dev, "c2");
> + if (IS_ERR(adev->c2_reset)) {
> + dev_err(adev->dev, "failed to get ADM0 C2 reset\n");
> + ret = PTR_ERR(adev->c2_reset);
> + goto err_disable_clks;
> + }
Please use devm_reset_control_get_exclusive(). Move these up before the
core_clk enable, and you can directly return the error.
regards
Philipp
Re: [PATCH v12 2/2] Add PWM fan controller driver for LGM SoC
Hi Rahul,
I just have some small nitpicks, see below:
On Wed, 2020-09-09 at 15:51 +0800, Rahul Tanwar wrote:
> Intel Lightning Mountain(LGM) SoC contains a PWM fan controller.
> This PWM controller does not have any other consumer, it is a
> dedicated PWM controller for fan attached to the system. Add
> driver for this PWM fan controller.
>
> Signed-off-by: Rahul Tanwar
> Reviewed-by: Andy Shevchenko
> ---
> drivers/pwm/Kconfig | 11 ++
> drivers/pwm/Makefile| 1 +
> drivers/pwm/pwm-intel-lgm.c | 253
>
> 3 files changed, 265 insertions(+)
> create mode 100644 drivers/pwm/pwm-intel-lgm.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 7dbcf6973d33..4949c51fe90b 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -232,6 +232,17 @@ config PWM_IMX_TPM
> To compile this driver as a module, choose M here: the module
> will be called pwm-imx-tpm.
>
> +config PWM_INTEL_LGM
> + tristate "Intel LGM PWM support"
> + depends on HAS_IOMEM
> + depends on (OF && X86) || COMPILE_TEST
> + select REGMAP_MMIO
> + help
> + Generic PWM fan controller driver for LGM SoC.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-intel-lgm.
> +
> config PWM_IQS620A
> tristate "Azoteq IQS620A PWM support"
> depends on MFD_IQS62X || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 2c2ba0a03557..e9431b151694 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_PWM_IMG) += pwm-img.o
> obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o
> obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o
> obj-$(CONFIG_PWM_IMX_TPM)+= pwm-imx-tpm.o
> +obj-$(CONFIG_PWM_INTEL_LGM) += pwm-intel-lgm.o
> obj-$(CONFIG_PWM_IQS620A)+= pwm-iqs620a.o
> obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
> obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o
> diff --git a/drivers/pwm/pwm-intel-lgm.c b/drivers/pwm/pwm-intel-lgm.c
> new file mode 100644
> index ..8e9f8cd3b7fb
> --- /dev/null
> +++ b/drivers/pwm/pwm-intel-lgm.c
> @@ -0,0 +1,253 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 Intel Corporation.
> + *
> + * Limitations:
> + * - The hardware supports fixed period which is dependent on 2/3 or 4
> + * wire fan mode.
> + * - Supports normal polarity. Does not support changing polarity.
> + * - When PWM is disabled, output of PWM will become 0(inactive). It doesn't
> + * keep track of running period.
> + * - When duty cycle is changed, PWM output may be a mix of previous setting
> + * and new setting for the first period. From second period, the output is
> + * based on new setting.
> + * - It is a dedicated PWM fan controller. There are no other consumers for
> + * this PWM controller.
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define LGM_PWM_FAN_CON0 0x0
> +#define LGM_PWM_FAN_EN_ENBIT(0)
> +#define LGM_PWM_FAN_EN_DIS 0x0
> +#define LGM_PWM_FAN_EN_MSK BIT(0)
> +#define LGM_PWM_FAN_MODE_2WIRE 0x0
> +#define LGM_PWM_FAN_MODE_MSK BIT(1)
> +#define LGM_PWM_FAN_DC_MSK GENMASK(23, 16)
> +
> +#define LGM_PWM_FAN_CON1 0x4
> +#define LGM_PWM_FAN_MAX_RPM_MSK GENMASK(15, 0)
> +
> +#define LGM_PWM_MAX_RPM (BIT(16) - 1)
> +#define LGM_PWM_DEFAULT_RPM 4000
> +#define LGM_PWM_MAX_DUTY_CYCLE (BIT(8) - 1)
> +
> +#define LGM_PWM_DC_BITS 8
> +
> +#define LGM_PWM_PERIOD_2WIRE_NS (40 * NSEC_PER_MSEC)
> +
> +struct lgm_pwm_chip {
> + struct pwm_chip chip;
> + struct regmap *regmap;
> + struct clk *clk;
> + struct reset_control *rst;
> + u32 period;
> +};
> +
> +static inline struct lgm_pwm_chip *to_lgm_pwm_chip(struct pwm_chip *chip)
> +{
> + return container_of(chip, struct lgm_pwm_chip, chip);
> +}
> +
> +static int lgm_pwm_enable(struct pwm_chip *chip, bool enable)
> +{
> + struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip);
> + struct regmap *regmap = pc->regmap;
> +
> + return regmap_update_bits(regmap, LGM_PWM_FAN_CON0, LGM_PWM_FAN_EN_MSK,
> + enable ? LGM_PWM_FAN_EN_EN :
> LGM_PWM_FAN_EN_DIS);
> +}
> +
> +static int lgm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip);
> + u32 duty_cycle, val;
> + int ret;
> +
> + /*
> + * The hardware only supports
> + * normal polarity and fixed period.
> + */
That comment could be put on a single line.
> + if (state->polarity != PWM_POLARITY_NORMAL || state->period <
> pc->period)
> + return -EINVAL;
> +
>
Re: [v4,3/4] reset-controller: ti: introduce a new reset handler
Hi Crystal,
On Fri, 2020-09-11 at 14:07 +0800, Crystal Guo wrote:
[...]
> Should I add the SoC-specific data as follows?
> This may also modify the ti original code, is it OK?
>
> + data->reset_data = of_device_get_match_data(>dev);
> +
> + list = of_get_property(np, data->reset_data->reset_bits, );
>
> +static const struct common_reset_data ti_reset_data = {
> + .reset_op_available = false,
> + .reset_bits = "ti, reset-bits",
^
That space doesn't belong there.
> +};
> +
> +static const struct common_reset_data mediatek_reset_data = {
> + .reset_op_available = true,
> + .reset_bits = "mediatek, reset-bits",
> +};
I understand Robs comments as meaning "ti,reset-bits" should have been
called "reset-bits" in the first place, and you shouldn't repeat adding
the vendor prefix, as that is implied by the compatible. So this should
probably be just "reset-bits".
Otherwise this looks like it should work.
regards
Philipp
Re: [v2,2/3] PCI: mediatek: Add new generation controller support
Hi Jianjun,
On Thu, 2020-09-10 at 11:45 +0800, Jianjun Wang wrote:
> MediaTek's PCIe host controller has three generation HWs, the new
> generation HW is an individual bridge, it supoorts Gen3 speed and
> up to 256 MSI interrupt numbers for multi-function devices.
>
> Add support for new Gen3 controller which can be found on MT8192.
>
> Signed-off-by: Jianjun Wang
> Acked-by: Ryder Lee
> ---
> drivers/pci/controller/Kconfig | 14 +
> drivers/pci/controller/Makefile |1 +
> drivers/pci/controller/pcie-mediatek-gen3.c | 1076 +++
> 3 files changed, 1091 insertions(+)
> create mode 100644 drivers/pci/controller/pcie-mediatek-gen3.c
>
[...]
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> b/drivers/pci/controller/pcie-mediatek-gen3.c
> new file mode 100644
> index ..f8c8bdf88d33
> --- /dev/null
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
[...]
> +static int mtk_pcie_power_up(struct mtk_pcie_port *port)
> +{
> + struct device *dev = port->dev;
> + int err;
> +
> + port->phy_reset = devm_reset_control_get_optional(dev, "phy-rst");
Please use devm_reset_control_get_optional_exclusive() instead.
> + if (PTR_ERR(port->phy_reset) == -EPROBE_DEFER)
> + return PTR_ERR(port->phy_reset);
This should be
if (IS_ERR(port->phy_reset))
return PTR_ERR(port->phy_reset);
there is no reason to continue if this throws -ENOMEM, for example.
regards
Philipp
Re: [PATCH 2/2] Add driver for Moortec MR75203 PVT controller
On Wed, 2020-09-09 at 14:52 +0800, Rahul Tanwar wrote:
> PVT controller (MR75203) is used to configure & control
> Moortec embedded analog IP which contains temprature
> sensor(TS), voltage monitor(VM) & process detector(PD)
> modules. Add driver to support MR75203 PVT controller.
>
> Signed-off-by: Rahul Tanwar
> ---
[...]
> +static int mr75203_probe(struct platform_device *pdev)
> +{
> + const struct hwmon_channel_info **pvt_info;
> + u32 ts_num, vm_num, pd_num, val, index, i;
> + struct device *dev = >dev;
> + u32 *temp_config, *in_config;
> + struct device *hwmon_dev;
> + struct pvt_device *pvt;
> + int ret;
> +
> + pvt = devm_kzalloc(dev, sizeof(*pvt), GFP_KERNEL);
> + if (!pvt)
> + return -ENOMEM;
> +
> + ret = pvt_get_regmap(pdev, "common");
> + if (ret)
> + return ret;
> +
> + pvt->rst = devm_reset_control_get(dev, NULL);
Please use devm_reset_control_get_exclusive().
regards
Philipp
Re: [PATCH] Revert "usb: dwc3: meson-g12a: fix shared reset control use"
On Thu, 2020-08-27 at 16:48 +0200, Amjad Ouled-Ameur wrote: > This reverts commit 7a410953d1fb4dbe91ffcfdee9cbbf889d19b0d7. > > This commit breaks USB on meson-gxl-s905x-libretech-cc. Reverting > the change solves the issue. > > In fact, according to the reset framework code, consumers must not use > reset_control_(de)assert() on shared reset lines when reset_control_reset > has been used, and vice-versa. > > Moreover, with this commit, usb is not guaranted to be reset since the > reset is likely to be initially deasserted. > > Reverting the commit will bring back the suspend warning mentioned in the > commit description. Nevertheless, a warning is much less critical than > breaking dwc3-meson-g12a USB completely. We will address the warning > issue in another way as a 2nd step. > > Signed-off-by: Amjad Ouled-Ameur > Reported-by: Jerome Brunet Acked-by: Philipp Zabel regards Philipp
Re: [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms
Hi Manish,
On Thu, 2020-08-27 at 00:14 +0530, Manish Narani wrote:
> Add a new driver for supporting Xilinx platforms. This driver handles
> the USB 3.0 PHY initialization and PIPE control & reset operations for
> ZynqMP platforms. This also handles the USB 2.0 PHY initialization and
> reset operations for Versal platforms.
>
> Signed-off-by: Manish Narani
> ---
[...]
> +static int dwc3_xlnx_rst_assert(struct reset_control *rstc)
> +{
> + unsigned long loop_time = msecs_to_jiffies(RST_TIMEOUT);
> + unsigned long timeout;
> +
> + reset_control_assert(rstc);
> +
> + /* wait until reset is asserted or timeout */
> + timeout = jiffies + loop_time;
> +
> + while (!time_after_eq(jiffies, timeout)) {
> + if (reset_control_status(rstc) > 0)
> + return 0;
> +
> + cpu_relax();
> + }
> +
> + return -ETIMEDOUT;
> +}
I think this should be done in the reset controller driver instead.
When reset_control_assert() is called with an exclusive reset control,
the reset line should be already asserted when the function returns.
> +
> +static int dwc3_xlnx_rst_deassert(struct reset_control *rstc)
> +{
> + unsigned long loop_time = msecs_to_jiffies(RST_TIMEOUT);
> + unsigned long timeout;
> +
> + reset_control_deassert(rstc);
> +
> + /* wait until reset is de-asserted or timeout */
> + timeout = jiffies + loop_time;
> + while (!time_after_eq(jiffies, timeout)) {
> + if (!reset_control_status(rstc))
> + return 0;
> +
> + cpu_relax();
> + }
> +
> + return -ETIMEDOUT;
> +}
Same as above, this belongs in the reset controller driver.
> +static int dwc3_xlnx_init_versal(struct dwc3_xlnx *priv_data)
> +{
> + struct device *dev = priv_data->dev;
> + int ret;
> +
> + dwc3_xlnx_mask_phy_rst(priv_data, false);
> +
> + /* Assert and De-assert reset */
> + ret = zynqmp_pm_reset_assert(VERSAL_USB_RESET_ID,
> + PM_RESET_ACTION_ASSERT);
> + if (ret < 0) {
> + dev_err(dev, "failed to assert Reset\n");
> + return ret;
> + }
> +
> + ret = zynqmp_pm_reset_assert(VERSAL_USB_RESET_ID,
> + PM_RESET_ACTION_RELEASE);
> + if (ret < 0) {
> + dev_err(dev, "failed to De-assert Reset\n");
> + return ret;
> + }
> +
> + dwc3_xlnx_mask_phy_rst(priv_data, true);
> +
> + return 0;
> +}
> +
> +static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
> +{
> + struct device *dev = priv_data->dev;
> + int ret;
> + u32 reg;
> +
> + priv_data->crst = devm_reset_control_get(dev, "usb_crst");
Please use devm_reset_control_get_exclusive() instead.
> + if (IS_ERR(priv_data->crst)) {
> + dev_err(dev, "failed to get %s reset signal\n", "usb_crst");
Consider using dev_err_probe() to hide -EPROBE_DEFER error values.
> + ret = PTR_ERR(priv_data->crst);
> + goto err;
> + }
> +
> + priv_data->hibrst = devm_reset_control_get(dev, "usb_hibrst");
> + if (IS_ERR(priv_data->hibrst)) {
> + dev_err(dev, "failed to get %s reset signal\n", "usb_hibrst");
> + ret = PTR_ERR(priv_data->hibrst);
> + goto err;
> + }
> +
> + priv_data->apbrst = devm_reset_control_get(dev, "usb_apbrst");
> + if (IS_ERR(priv_data->apbrst)) {
> + dev_err(dev, "failed to get %s reset signal\n", "usb_apbrst");
> + ret = PTR_ERR(priv_data->apbrst);
> + goto err;
> + }
Same as above for the other two reset controls.
> + priv_data->usb3_phy = devm_phy_get(dev, "usb3-phy");
> +
> + ret = dwc3_xlnx_rst_assert(priv_data->crst);
> + if (ret < 0) {
> + dev_err(dev, "%s: %d: Failed to assert reset\n",
> + __func__, __LINE__);
> + goto err;
> + }
> +
> + ret = dwc3_xlnx_rst_assert(priv_data->hibrst);
> + if (ret < 0) {
> + dev_err(dev, "%s: %d: Failed to assert reset\n",
> + __func__, __LINE__);
> + goto err;
> + }
> +
> + ret = dwc3_xlnx_rst_assert(priv_data->apbrst);
> + if (ret < 0) {
> + dev_err(dev, "%s: %d: Failed to assert reset\n",
> + __func__, __LINE__);
> + goto err;
> + }
> +
> + ret = phy_init(priv_data->usb3_phy);
> + if (ret < 0) {
> + phy_exit(priv_data->usb3_phy);
> + goto err;
> + }
> +
> + ret = dwc3_xlnx_rst_deassert(priv_data->apbrst);
> + if (ret < 0) {
> + dev_err(dev, "%s: %d: Failed to release reset\n",
> + __func__, __LINE__);
> + goto err;
> + }
> +
> + /* Set PIPE power present signal */
> + writel(PIPE_POWER_ON, priv_data->regs + PIPE_POWER_OFFSET);
> +
> + /* Clear PIPE CLK signal */
> + writel(PIPE_CLK_OFF, priv_data->regs +
Re: [PATCH v2 11/17] clk: imx: Add blk_ctrl combo driver
On Tue, 2020-08-25 at 21:30 +0300, Abel Vesa wrote: [...] > > if (assert) > > pm_runtime_get_sync(); > > spin_lock_irqsave(); > > /* ... */ > > spin_unlock_irqrestore(); > > if (assert && asserted_before) > > pm_runtime_put(); > > > > On a second thought this doesn't work because, for the first assertion, > the runtime put will never be called, if the asserted_before does not count > the current assertion. I'm not sure I follow. The first assert will increment device usage 0 -> 1, all others asserts will just temporarily increment and decrement 1 -> 2 -> 1. Isn't this just missing one if (!assert && !asserted_after) pm_runtime_put() to do the last deassert 1 -> 0 transition? > If it counts the current assertion, then every assertion > will end with runtime put. None of these options work here. > > How about the following: > > if (assert && !test_and_set_bit(1, >rst_hws[id].asserted)) > pm_runtime_get_sync(rcdev->dev); > > spin_lock_irqsave(>lock, flags); > > reg = readl(reg_addr); > if (assert) > writel(reg & ~(mask << shift), reg_addr); > else > writel(reg | (mask << shift), reg_addr); > > spin_unlock_irqrestore(>lock, flags); > > if (!assert && test_and_clear_bit(1, >rst_hws[id].asserted)) > pm_runtime_put(rcdev->dev); > > This would only call the get_sync/put once for each reset bit. Yes, that should work. I think it is a much better idea, no more looping through the entire reset control array. regards Philipp
Re: [PATCH v2 11/17] clk: imx: Add blk_ctrl combo driver
On Tue, 2020-08-25 at 14:24 +0300, Abel Vesa wrote:
[...]
> > > +static int imx_blk_ctrl_reset_set(struct reset_controller_dev *rcdev,
> > > + unsigned long id, bool assert)
> > > +{
> > > + struct imx_blk_ctrl_drvdata *drvdata = container_of(rcdev,
> > > + struct imx_blk_ctrl_drvdata, rcdev);
> > > + unsigned int offset = drvdata->rst_hws[id].offset;
> > > + unsigned int shift = drvdata->rst_hws[id].shift;
> > > + unsigned int mask = drvdata->rst_hws[id].mask;
> > > + void __iomem *reg_addr = drvdata->base + offset;
> > > + unsigned long flags;
> > > + unsigned int asserted_before = 0, asserted_after = 0;
> > > + u32 reg;
> > > + int i;
> > > +
> > > + spin_lock_irqsave(>lock, flags);
> > > +
> > > + for (i = 0; i < drvdata->rcdev.nr_resets; i++)
> > > + if (drvdata->rst_hws[i].asserted)
> > > + asserted_before++;
> > > +
> > > + if (asserted_before == 0 && assert)
> > > + pm_runtime_get(rcdev->dev);
> >
> > Shouldn't that be pm_runtime_get_sync() ?
> >
> > I would do that unconditionally before locking drvdata->lock and then
> > drop unnecessary refcounts afterwards.
> >
>
> I thought we already discussed this on the last's version thread.
This is about something different. pm_runtime_get() just queues the
device to be enabled at a later point, but I presume you want to have it
enabled before writing to its registers. (The question here is can you
write to the registers, and have the device update its internal state,
while the power domain is disabled?)
Either way, if you want the reset to be asserted after the function
returns (as is required by the reset API), as I understand it, you have
to make sure that the power domain is activated before the function
returns.
Therefore pm_runtime_get_sync() is required instead of pm_runtime_get(),
and that must be called outside of the spin locked section. My
suggestion would be:
if (assert)
pm_runtime_get_sync();
spin_lock_irqsave();
/* ... */
spin_unlock_irqrestore();
if (assert && asserted_before)
pm_runtime_put();
unless the following might be an issue:
> > > +
> > > + if (assert) {
> > > + reg = readl(reg_addr);
> > > + writel(reg & ~(mask << shift), reg_addr);
> > > + drvdata->rst_hws[id].asserted = true;
> > > + } else {
> > > + reg = readl(reg_addr);
> > > + writel(reg | (mask << shift), reg_addr);
Could this cause problems if the power domain is already disabled? If
so, it would be best to either temporarily enable power, or to skip the
register writes if asserted_before == 0 && !assert.
> > > + drvdata->rst_hws[id].asserted = false;
> > > + }
> > > +
> > > + for (i = 0; i < drvdata->rcdev.nr_resets; i++)
> > > + if (drvdata->rst_hws[i].asserted)
> > > + asserted_after++;
> > > +
> > > + if (asserted_before == 1 && asserted_after == 0)
> > > + pm_runtime_put(rcdev->dev);
> > > +
> > > + spin_unlock_irqrestore(>lock, flags);
> > > +
> > > + return 0;
> > > +}
regards
Philipp
Re: [PATCH v2 11/17] clk: imx: Add blk_ctrl combo driver
On Fri, 2020-08-14 at 15:09 +0300, Abel Vesa wrote:
> On i.MX8MP, there is a new type of IP which is called BLK_CTRL in
> RM and usually is comprised of some GPRs that are considered too
> generic to be part of any dedicated IP from that specific subsystem.
>
> In general, some of the GPRs have some clock bits, some have reset bits,
> so in order to be able to use the imx clock API, this needs to be
> in a clock driver. From there it can use the reset controller API and
> leave the rest to the syscon.
>
> This driver is intended to work with the following BLK_CTRL IPs found in
> i.MX8MP (but it might be reused by the future i.MX platforms that
> have this kind of IP in their design):
> - Audio
> - Media
> - HDMI
>
> Signed-off-by: Abel Vesa
> ---
> drivers/clk/imx/Makefile | 2 +-
> drivers/clk/imx/clk-blk-ctrl.c | 327
> +
> drivers/clk/imx/clk-blk-ctrl.h | 81 ++
> 3 files changed, 409 insertions(+), 1 deletion(-)
> create mode 100644 drivers/clk/imx/clk-blk-ctrl.c
> create mode 100644 drivers/clk/imx/clk-blk-ctrl.h
>
> diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
> index 928f874c..7afe1df 100644
> --- a/drivers/clk/imx/Makefile
> +++ b/drivers/clk/imx/Makefile
> @@ -27,7 +27,7 @@ obj-$(CONFIG_MXC_CLK_SCU) += \
>
> obj-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o
> obj-$(CONFIG_CLK_IMX8MN) += clk-imx8mn.o
> -obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o
> +obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o clk-blk-ctrl.o
> obj-$(CONFIG_CLK_IMX8MQ) += clk-imx8mq.o
> obj-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o
>
> diff --git a/drivers/clk/imx/clk-blk-ctrl.c b/drivers/clk/imx/clk-blk-ctrl.c
> new file mode 100644
> index ..1672646
> --- /dev/null
> +++ b/drivers/clk/imx/clk-blk-ctrl.c
> @@ -0,0 +1,327 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2020 NXP.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "clk.h"
> +#include "clk-blk-ctrl.h"
> +
> +struct reset_hw {
> + u32 offset;
> + u32 shift;
> + u32 mask;
> + bool asserted;
> +};
> +
> +struct pm_safekeep_info {
> + uint32_t *regs_values;
> + uint32_t *regs_offsets;
> + uint32_t regs_num;
> +};
> +
> +struct imx_blk_ctrl_drvdata {
> + void __iomem *base;
> + struct reset_controller_dev rcdev;
> + struct reset_hw *rst_hws;
> + struct pm_safekeep_info pm_info;
> +
> + spinlock_t lock;
> +};
> +
> +static int imx_blk_ctrl_reset_set(struct reset_controller_dev *rcdev,
> + unsigned long id, bool assert)
> +{
> + struct imx_blk_ctrl_drvdata *drvdata = container_of(rcdev,
> + struct imx_blk_ctrl_drvdata, rcdev);
> + unsigned int offset = drvdata->rst_hws[id].offset;
> + unsigned int shift = drvdata->rst_hws[id].shift;
> + unsigned int mask = drvdata->rst_hws[id].mask;
> + void __iomem *reg_addr = drvdata->base + offset;
> + unsigned long flags;
> + unsigned int asserted_before = 0, asserted_after = 0;
> + u32 reg;
> + int i;
> +
> + spin_lock_irqsave(>lock, flags);
> +
> + for (i = 0; i < drvdata->rcdev.nr_resets; i++)
> + if (drvdata->rst_hws[i].asserted)
> + asserted_before++;
> +
> + if (asserted_before == 0 && assert)
> + pm_runtime_get(rcdev->dev);
Shouldn't that be pm_runtime_get_sync() ?
I would do that unconditionally before locking drvdata->lock and then
drop unnecessary refcounts afterwards.
> +
> + if (assert) {
> + reg = readl(reg_addr);
> + writel(reg & ~(mask << shift), reg_addr);
> + drvdata->rst_hws[id].asserted = true;
> + } else {
> + reg = readl(reg_addr);
> + writel(reg | (mask << shift), reg_addr);
> + drvdata->rst_hws[id].asserted = false;
> + }
> +
> + for (i = 0; i < drvdata->rcdev.nr_resets; i++)
> + if (drvdata->rst_hws[i].asserted)
> + asserted_after++;
> +
> + if (asserted_before == 1 && asserted_after == 0)
> + pm_runtime_put(rcdev->dev);
> +
> + spin_unlock_irqrestore(>lock, flags);
> +
> + return 0;
> +}
regards
Philipp
Re: [PATCH v1] ata: ahci_brcm: Fix use of BCM7216 reset controller
On Mon, 2020-08-24 at 16:40 -0400, Jim Quinlan wrote:
> From: Jim Quinlan
>
> A reset controller "rescal" is shared between the AHCI driver and the PCIe
> driver for the BrcmSTB 7216 chip. Use
> devm_reset_control_get_optional_shared() to handle this sharing.
>
> Fixes: 272ecd60a636 ("ata: ahci_brcm: BCM7216 reset is self de-asserting")
> Fixes: c345ec6a50e9 ("ata: ahci_brcm: Support BCM7216 reset controller name")
> Signed-off-by: Jim Quinlan
> ---
> drivers/ata/ahci_brcm.c | 11 +++
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
> index 6853dbb4131d..d6115bc04b09 100644
> --- a/drivers/ata/ahci_brcm.c
> +++ b/drivers/ata/ahci_brcm.c
> @@ -428,7 +428,6 @@ static int brcm_ahci_probe(struct platform_device *pdev)
> {
> const struct of_device_id *of_id;
> struct device *dev = >dev;
> - const char *reset_name = NULL;
> struct brcm_ahci_priv *priv;
> struct ahci_host_priv *hpriv;
> struct resource *res;
> @@ -452,11 +451,10 @@ static int brcm_ahci_probe(struct platform_device *pdev)
>
> /* Reset is optional depending on platform and named differently */
> if (priv->version == BRCM_SATA_BCM7216)
> - reset_name = "rescal";
> + priv->rcdev =
> devm_reset_control_get_optional_shared(>dev, "rescal");
> else
> - reset_name = "ahci";
> + priv->rcdev = devm_reset_control_get_optional(>dev,
> "ahci");
I think it would be cleaner to use two separate reset control handles
here. It is hard to reason about what the code does when the reset
control is shared on one platform and exclusive on the other.
> - priv->rcdev = devm_reset_control_get_optional(>dev, reset_name);
> if (IS_ERR(priv->rcdev))
> return PTR_ERR(priv->rcdev);
>
> @@ -479,10 +477,7 @@ static int brcm_ahci_probe(struct platform_device *pdev)
> break;
> }
>
> - if (priv->version == BRCM_SATA_BCM7216)
> - ret = reset_control_reset(priv->rcdev);
I think we might have a similar issue currently with
"usb: dwc3: meson-g12a: fix shared reset control use", where two IP
cores try to share a pulsed reset line.
> - else
> - ret = reset_control_deassert(priv->rcdev);
> + ret = reset_control_deassert(priv->rcdev);
Isn't the shared 'rescal' reset a triggered reset pulse? Looking at the
reset-brcmstb-rescal reset controller driver, without reset line level
control implemented, this will turn into a no-op for BCM7216. Yes, the
reset line will be deasserted after this call, but there is no guarantee
that the reset line was ever pulsed.
regards
Philipp
Re: [PATCH 4.9 196/212] gpu: ipu-v3: image-convert: Combine rotate/no-rotate irq handlers
On Fri, 2020-08-21 at 09:34 +0200, Greg Kroah-Hartman wrote:
> On Fri, Aug 21, 2020 at 09:10:30AM +0200, Philipp Zabel wrote:
> > Hi,
> >
> > On Fri, 2020-08-21 at 09:02 +0200, Pavel Machek wrote:
> > > Hi!
> > >
> > > > From: Steve Longerbeam
> > > >
> > > > [ Upstream commit 0f6245f42ce9b7e4d20f2cda8d5f12b55a44d7d1 ]
> > > >
> > > > Combine the rotate_irq() and norotate_irq() handlers into a single
> > > > eof_irq() handler.
> > >
> > > AFAICT this is preparation for next patch, not a backfix. And actual
> > > fix patch is not there for 4.19, so this can be dropped, too.
^^ 4.9
> >
> > You are right, this patch is preparation for commit 0f6245f42ce9 ("gpu:
> > ipu-v3: image-convert: Wait for all EOFs before completing a tile").
>
> Which is included in this patch series...
It didn't hit my inbox for the v4.9 series, I can't see it on lore
either:
https://lore.kernel.org/stable/20200820091602.251285...@linuxfoundation.org/
regards
Philipp
Re: [PATCH 4.9 196/212] gpu: ipu-v3: image-convert: Combine rotate/no-rotate irq handlers
Hi,
On Fri, 2020-08-21 at 09:02 +0200, Pavel Machek wrote:
> Hi!
>
> > From: Steve Longerbeam
> >
> > [ Upstream commit 0f6245f42ce9b7e4d20f2cda8d5f12b55a44d7d1 ]
> >
> > Combine the rotate_irq() and norotate_irq() handlers into a single
> > eof_irq() handler.
>
> AFAICT this is preparation for next patch, not a backfix. And actual
> fix patch is not there for 4.19, so this can be dropped, too.
You are right, this patch is preparation for commit 0f6245f42ce9 ("gpu:
ipu-v3: image-convert: Wait for all EOFs before completing a tile").
regards
Philipp
Re: [PATCH 2/7] media: coda: no need to check return value of debugfs_create functions
On Tue, 2020-08-18 at 15:36 +0200, Greg Kroah-Hartman wrote:
> When calling debugfs functions, there is no need to ever check the
> return value. The function can work or not, but the code logic should
> never do something different based on this.
>
> Cc: Philipp Zabel
> Cc: Mauro Carvalho Chehab
> Cc: linux-me...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman
Thank you,
Reviewed-by: Philipp Zabel
regards
Philipp
> ---
> drivers/media/platform/coda/coda-common.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/media/platform/coda/coda-common.c
> b/drivers/media/platform/coda/coda-common.c
> index 3ab3d976d8ca..9020be29d8f2 100644
> --- a/drivers/media/platform/coda/coda-common.c
> +++ b/drivers/media/platform/coda/coda-common.c
> @@ -1937,9 +1937,6 @@ int coda_alloc_aux_buf(struct coda_dev *dev, struct
> coda_aux_buf *buf,
> buf->blob.size = size;
> buf->dentry = debugfs_create_blob(name, 0644, parent,
> >blob);
> - if (!buf->dentry)
> - dev_warn(dev->dev,
> - "failed to create debugfs entry %s\n", name);
> }
>
> return 0;
> @@ -3211,8 +3208,6 @@ static int coda_probe(struct platform_device *pdev)
> ida_init(>ida);
>
> dev->debugfs_root = debugfs_create_dir("coda", NULL);
> - if (!dev->debugfs_root)
> - dev_warn(>dev, "failed to create debugfs root\n");
>
> /* allocate auxiliary per-device buffers for the BIT processor */
> if (dev->devtype->product == CODA_DX6) {
Re: [PATCH 3/3] mmc: mediatek: add optional module reset property
On Wed, 2020-08-12 at 17:37 +0800, Wenbin Mei wrote:
> This patch adds a optional reset management for msdc.
> Sometimes the bootloader does not bring msdc register
> to default state, so need reset the msdc controller.
>
> Signed-off-by: Wenbin Mei
> ---
> drivers/mmc/host/mtk-sd.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index 39e7fc54c438..2b243c03c9b2 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -22,6 +22,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -434,6 +435,7 @@ struct msdc_host {
> struct msdc_save_para save_para; /* used when gate HCLK */
> struct msdc_tune_para def_tune_para; /* default tune setting */
> struct msdc_tune_para saved_tune_para; /* tune result of CMD21/CMD19 */
> + struct reset_control *reset;
> };
>
> static const struct mtk_mmc_compatible mt8135_compat = {
> @@ -1516,6 +1518,12 @@ static void msdc_init_hw(struct msdc_host *host)
> u32 val;
> u32 tune_reg = host->dev_comp->pad_tune_reg;
>
> + if (!IS_ERR(host->reset)) {
> + reset_control_assert(host->reset);
> + usleep_range(10, 50);
> + reset_control_deassert(host->reset);
> + }
> +
This should be:
if (host->reset) {
reset_control_assert(host->reset);
usleep_range(10, 50);
reset_control_deassert(host->reset);
}
> /* Configure to MMC/SD mode, clock free running */
> sdr_set_bits(host->base + MSDC_CFG, MSDC_CFG_MODE | MSDC_CFG_CKPDN);
>
> @@ -2273,6 +2281,11 @@ static int msdc_drv_probe(struct platform_device *pdev)
> if (IS_ERR(host->src_clk_cg))
> host->src_clk_cg = NULL;
>
> + host->reset = devm_reset_control_get_optional_exclusive(>dev,
> + "hrst");
> + if (PTR_ERR(host->reset) == -EPROBE_DEFER)
> + return PTR_ERR(host->reset);
> +
This should be:
host->reset = devm_reset_control_get_optional_exclusive(>dev,
"hrst");
if (IS_ERR(host->reset))
return PTR_ERR(host->reset);
If the reset is configured in DT then it should be used, even if the
reset driver is loaded later.
If the DT does not contain the reset-names = "hrst" property at all,
devm_reset_control_get_optional_*() will return NULL.
With these two changes,
Reviewed-by: Philipp Zabel
regards
Philipp
Re: [PATCH 1/3] mmc: dt-bindings: Add resets/reset-names for Mediatek MMC bindings
On Wed, 2020-08-12 at 17:37 +0800, Wenbin Mei wrote: > Add description for resets/reset-names. > > Signed-off-by: Wenbin Mei > --- > Documentation/devicetree/bindings/mmc/mtk-sd.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mmc/mtk-sd.txt > b/Documentation/devicetree/bindings/mmc/mtk-sd.txt > index 8a532f4453f2..35da72de7aac 100644 > --- a/Documentation/devicetree/bindings/mmc/mtk-sd.txt > +++ b/Documentation/devicetree/bindings/mmc/mtk-sd.txt > @@ -49,6 +49,8 @@ Optional properties: >error caused by stop clock(fifo full) >Valid range = [0:0x7]. if not present, default value is 0. >applied to compatible "mediatek,mt2701-mmc". > +- resets: Phandle and reset specifier pair to softreset line of MSDC IP. > +- reset-names: Reset names for MSDC. I think the reset-names documentation should mention the actual value the driver should look for, "hrst". regards Philipp
Re: [v2,3/6] dt-binding: reset-controller: ti: add generic-reset to compatible
On Mon, 2020-08-03 at 14:15 +0800, Crystal Guo wrote: > The TI syscon reset controller provides a common reset management, > and should be suitable for other SOCs. Add compatible "generic-reset", > which denotes to use a common reset-controller driver. > > Signed-off-by: Crystal Guo > --- > Documentation/devicetree/bindings/reset/ti-syscon-reset.txt | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt > b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt > index d551161ae785..e36d3631eab2 100644 > --- a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt > +++ b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt > @@ -25,6 +25,7 @@ Required properties: > "ti,k2l-pscrst" > "ti,k2hk-pscrst" > "ti,syscon-reset" > + "generic-reset", "ti,syscon-reset" > - #reset-cells : Should be 1. Please see the reset consumer > node below > for usage details > - ti,reset-bits : Contains the reset control register information > -- > 2.18.0 My understanding is that it would be better to add a mtk specific compatible instead of adding this "generic-reset", especially since we can't guarantee this binding will be considered generic in the future. I think there is nothing wrong with specifying compatible = "mtk,your-compatible", "ti,syscon-reset"; in your device tree if your hardware is indeed compatible with the specified "ti,syscon-reset" binding, but I may be wrong: Therefore, please add devicet...@vger.kernel.org to Cc: for binding changes. regards Philipp
Re: [v2,4/6] reset-controller: ti: introduce a new reset handler
On Mon, 2020-08-03 at 14:15 +0800, Crystal Guo wrote:
> Add ti_syscon_reset() to integrate assert and deassert together.
> If some modules need do serialized assert and deassert operations
> to reset itself, reset_control_reset can be called for convenience.
>
> Change-Id: I9046992b115a46f3594de57fa89c6a2de9957d49
> ---
> drivers/reset/reset-ti-syscon.c | 20
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
> index a2635c21db7f..1c74bcb9a6c3 100644
> --- a/drivers/reset/reset-ti-syscon.c
> +++ b/drivers/reset/reset-ti-syscon.c
> @@ -56,6 +56,7 @@ struct ti_syscon_reset_data {
> struct regmap *regmap;
> struct ti_syscon_reset_control *controls;
> unsigned int nr_controls;
> + bool assert_deassert_together;
> };
>
> #define to_ti_syscon_reset_data(rcdev) \
> @@ -158,10 +159,24 @@ static int ti_syscon_reset_status(struct
> reset_controller_dev *rcdev,
> !(control->flags & STATUS_SET);
> }
>
> +static int ti_syscon_reset(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct ti_syscon_reset_data *data = to_ti_syscon_reset_data(rcdev);
> +
> + if (data->assert_deassert_together) {
> + ti_syscon_reset_assert(rcdev, id);
> + return ti_syscon_reset_deassert(rcdev, id);
Even if your hardware engineers guarantee that no delays are necessary
between assert and deassert for any peripheral used with your reset
controller that may use this function, this is not generic.
I wonder if it would be better to define a minimum delay similarly to
reset-simple [1].
[1]
https://lore.kernel.org/lkml/be2cecb2654e68385561a15df7967c7723d5531d.1590594512.git-series.max...@cerno.tech/
> + } else {
> + return -ENOTSUPP;
> + }
> +}
> +
> static const struct reset_control_ops ti_syscon_reset_ops = {
> .assert = ti_syscon_reset_assert,
> .deassert = ti_syscon_reset_deassert,
> .status = ti_syscon_reset_status,
> + .reset = ti_syscon_reset,
> };
>
> static int ti_syscon_reset_probe(struct platform_device *pdev)
> @@ -204,6 +219,11 @@ static int ti_syscon_reset_probe(struct platform_device
> *pdev)
> controls[i].flags = be32_to_cpup(list++);
> }
>
> + if (of_property_read_bool(np, "assert-deassert-together"))
> + data->assert_deassert_together = true;
> + else
> + data->assert_deassert_together = false;
> +
This could just be assigned directly, but as said above, I think it
might be better to have a reset-duration-us property or similar.
regards
Philipp
Re: [v2,5/6] reset-controller: ti: Introduce force-update method
Hi Crystal,
On Mon, 2020-08-03 at 14:15 +0800, Crystal Guo wrote:
> Introduce force-update method for assert and deassert interface,
> which force the write operation in case the read already happens
> to return the correct value.
>
> Signed-off-by: Crystal Guo
Added Suman and Andrew for confirmation: I think writing unconditionally
can't break any existing user. Just changing to regmap_write_bits()
instead of adding the update-force property as in v1 should be fine.
regards
Philipp
> ---
> drivers/reset/reset-ti-syscon.c | 15 +--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
> index 1c74bcb9a6c3..f4baf78afd14 100644
> --- a/drivers/reset/reset-ti-syscon.c
> +++ b/drivers/reset/reset-ti-syscon.c
> @@ -57,6 +57,7 @@ struct ti_syscon_reset_data {
> struct ti_syscon_reset_control *controls;
> unsigned int nr_controls;
> bool assert_deassert_together;
> + bool update_force;
> };
>
> #define to_ti_syscon_reset_data(rcdev) \
> @@ -90,7 +91,10 @@ static int ti_syscon_reset_assert(struct
> reset_controller_dev *rcdev,
> mask = BIT(control->assert_bit);
> value = (control->flags & ASSERT_SET) ? mask : 0x0;
>
> - return regmap_update_bits(data->regmap, control->assert_offset, mask,
> value);
> + if (data->update_force)
> + return regmap_write_bits(data->regmap, control->assert_offset,
> mask, value);
> + else
> + return regmap_update_bits(data->regmap, control->assert_offset,
> mask, value);
> }
>
> /**
> @@ -121,7 +125,10 @@ static int ti_syscon_reset_deassert(struct
> reset_controller_dev *rcdev,
> mask = BIT(control->deassert_bit);
> value = (control->flags & DEASSERT_SET) ? mask : 0x0;
>
> - return regmap_update_bits(data->regmap, control->deassert_offset, mask,
> value);
> + if (data->update_force)
> + return regmap_write_bits(data->regmap,
> control->deassert_offset, mask, value);
> + else
> + return regmap_update_bits(data->regmap,
> control->deassert_offset, mask, value);
> }
>
> /**
> @@ -223,6 +230,10 @@ static int ti_syscon_reset_probe(struct platform_device
> *pdev)
> data->assert_deassert_together = true;
> else
> data->assert_deassert_together = false;
> + if (of_property_read_bool(np, "update-force"))
> + data->update_force = true;
> + else
> + data->update_force = false;
>
> data->rcdev.ops = _syscon_reset_ops;
> data->rcdev.owner = THIS_MODULE;
> --
> 2.18.0
Re: [PATCH 2/2] reset: imx7: add the cm4 reset for i.MX8MQ
On Thu, 2020-07-30 at 14:46 +0800, peng@nxp.com wrote: > From: Peng Fan > > Add the cm4 reset used by the remoteproc driver > > Signed-off-by: Peng Fan Thank you, both applied to reset/next. regards Philipp
Re: [PATCH 11/17] clk: imx: Add blk_ctrl combo driver
On Thu, 2020-07-30 at 11:55 +0300, Abel Vesa wrote:
> On 20-07-29 14:46:28, Philipp Zabel wrote:
> > Hi Abel,
> >
> > On Wed, 2020-07-29 at 15:07 +0300, Abel Vesa wrote:
> > > On i.MX8MP, there is a new type of IP which is called BLK_CTRL in
>
> [...]
>
> > > +
> > > +static int imx_blk_ctrl_reset_set(struct reset_controller_dev *rcdev,
> > > + unsigned long id, bool assert)
> > > +{
> > > + struct imx_blk_ctrl_drvdata *drvdata = container_of(rcdev,
> > > + struct imx_blk_ctrl_drvdata, rcdev);
> > > + unsigned int offset = drvdata->rst_hws[id].offset;
> > > + unsigned int shift = drvdata->rst_hws[id].shift;
> > > + unsigned int mask = drvdata->rst_hws[id].mask;
> > > + void __iomem *reg_addr = drvdata->base + offset;
> > > + unsigned long flags;
> > > + u32 reg;
> > > +
> > > + if (assert) {
> > > + pm_runtime_get_sync(rcdev->dev);
> > > + spin_lock_irqsave(>lock, flags);
> > > + reg = readl(reg_addr);
> > > + writel(reg & ~(mask << shift), reg_addr);
> > > + spin_unlock_irqrestore(>lock, flags);
> > > + } else {
> > > + spin_lock_irqsave(>lock, flags);
> > > + reg = readl(reg_addr);
> > > + writel(reg | (mask << shift), reg_addr);
> > > + spin_unlock_irqrestore(>lock, flags);
> > > + pm_runtime_put(rcdev->dev);
> >
> > This still has the issue of potentially letting exclusive reset control
> > users break the device usage counter.
> >
> > Also shared reset control users start with deassert(), and you end probe
> > with pm_runtime_put(), so the first shared reset control user that
> > deasserts its reset will decrement the dev->power.usage_count to -1 ?
> > For multiple resets being initially deasserted this would decrement
> > multiple times.
> >
> > I think you'll have to track the (number of) asserted reset bits in this
> > reset controller and limit when to call pm_runtime_get/put_sync().
> >
>
> Yes, you're right.
>
> I'll add a mask, and for each assert, the according bit will get set, and
> for each deasssert the same bit will get cleared.
> And when the mask has at least one bit set, the pm_runtime_get gets called
^ When the mask was 0 before but now has a bit set.
> and when the mask is 0, the pm_runtime_put_sync will be called.
^ When the mask had a bit set but now is 0.
> Does that sound OK ?
And the mask starts out as 0, as after the pm_runtime_put() in probe all
reset lines are deasserted?
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int imx_blk_ctrl_reset_reset(struct reset_controller_dev *rcdev,
> > > +unsigned long id)
> > > +{
> > > + imx_blk_ctrl_reset_set(rcdev, id, true);
> > > + return imx_blk_ctrl_reset_set(rcdev, id, false);
> >
> > Does this work for all peripherals? Are there none that require the
> > reset line to be asserted for a certain number of bus clocks or similar?
>
> As of now, there is no user that calls reset. All the users call the assert
> and then deassert. As for the number of clocks for reset, I'll try to have a
> chat to the HW design team and then come back with the information.
Ok. If this is not required or can't be guaranteed to work, it may be
better to just leave it out.
regards
Philipp
Re: [PATCH V3 3/3] pci: imx: Select RESET_IMX7 by default
Hi Anson, On Thu, 2020-07-30 at 02:11 +, Anson Huang wrote: > Hi, Philipp/Rob > > > Subject: Re: [PATCH V3 3/3] pci: imx: Select RESET_IMX7 by default > > > > On Wed, 2020-07-29 at 09:26 -0600, Rob Herring wrote: > > > On Mon, Jul 20, 2020 at 8:26 AM Anson Huang > > wrote: > > > > i.MX7 reset driver now supports module build and it is no longer > > > > built in by default, so i.MX PCI driver needs to select it > > > > explicitly due to it is NOT supporting loadable module currently. > > > > > > > > Signed-off-by: Anson Huang > > > > --- > > > > No change. > > > > --- > > > > drivers/pci/controller/dwc/Kconfig | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/drivers/pci/controller/dwc/Kconfig > > > > b/drivers/pci/controller/dwc/Kconfig > > > > index 044a376..bcf63ce 100644 > > > > --- a/drivers/pci/controller/dwc/Kconfig > > > > +++ b/drivers/pci/controller/dwc/Kconfig > > > > @@ -90,6 +90,7 @@ config PCI_EXYNOS > > > > > > > > config PCI_IMX6 > > > > bool "Freescale i.MX6/7/8 PCIe controller" > > > > + select RESET_IMX7 > > > > > > This will break as select will not cause all of RESET_IMX7's > > > dependencies to be met. It also doesn't scale. Are you going to do the > > > same thing for clocks, pinctrl, gpio, etc.? > > > > > > You should make the PCI driver work as a module. > > > > Oh, also PCI_IMX6 is used on (surprise) i.MX6, which doesn't need > > RESET_IMX7 at all. > > > > How about hiding the RESET_IMX7 option and setting it default y if > > PCI_IMX6 is enabled, as an interim solution? > > Like below, RESET_IMX7 is already default y when SOC_IMX7D, now added > PCI_IMX6, > let me know if it is OK for you, then I will send new patch for review. > > +++ b/drivers/reset/Kconfig > @@ -68,7 +68,7 @@ config RESET_IMX7 > tristate "i.MX7/8 Reset Driver" I was thinking something like tristate "i.MX7/8 Reset Driver" if COMPILE_TEST || !PCI_IMX6 > depends on HAS_IOMEM > depends on SOC_IMX7D || (ARM64 && ARCH_MXC) || COMPILE_TEST > - default y if SOC_IMX7D > + default y if (SOC_IMX7D || PCI_IMX6) Yes, although without the above I think it could still be disabled manually or via oldconfig. regards Philipp
Re: [PATCH V3 3/3] pci: imx: Select RESET_IMX7 by default
On Wed, 2020-07-29 at 09:26 -0600, Rob Herring wrote: > On Mon, Jul 20, 2020 at 8:26 AM Anson Huang wrote: > > i.MX7 reset driver now supports module build and it is no longer > > built in by default, so i.MX PCI driver needs to select it explicitly > > due to it is NOT supporting loadable module currently. > > > > Signed-off-by: Anson Huang > > --- > > No change. > > --- > > drivers/pci/controller/dwc/Kconfig | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/pci/controller/dwc/Kconfig > > b/drivers/pci/controller/dwc/Kconfig > > index 044a376..bcf63ce 100644 > > --- a/drivers/pci/controller/dwc/Kconfig > > +++ b/drivers/pci/controller/dwc/Kconfig > > @@ -90,6 +90,7 @@ config PCI_EXYNOS > > > > config PCI_IMX6 > > bool "Freescale i.MX6/7/8 PCIe controller" > > + select RESET_IMX7 > > This will break as select will not cause all of RESET_IMX7's > dependencies to be met. It also doesn't scale. Are you going to do the > same thing for clocks, pinctrl, gpio, etc.? > > You should make the PCI driver work as a module. Oh, also PCI_IMX6 is used on (surprise) i.MX6, which doesn't need RESET_IMX7 at all. How about hiding the RESET_IMX7 option and setting it default y if PCI_IMX6 is enabled, as an interim solution? regards Philipp
Re: [PATCH V3 1/3] reset: imx7: Support module build
On Tue, 2020-07-28 at 11:53 +0100, Lorenzo Pieralisi wrote: > On Fri, Jul 24, 2020 at 10:03:11AM +0200, Philipp Zabel wrote: > > On Mon, 2020-07-20 at 22:21 +0800, Anson Huang wrote: > > > Use module_platform_driver(), add module device table, author, > > > description and license to support module build, and > > > CONFIG_RESET_IMX7 is changed to default 'y' ONLY for i.MX7D, > > > other platforms need to select it in defconfig. > > > > > > Signed-off-by: Anson Huang > > > --- > > > Changes since V2: > > > - use module_platform_driver() instead of builtin_platform_driver(). > > > > Thank you, applied to reset/next. > > I think you should pick up patch (3) as well please if PCI_IMX6 > maintainers ACK it - merging just patch(1) will trigger regressions > AFAICS. Thank you for raising this, I'll put these patches on hold until the PCI_IMX6 issue is resolved. regards Philipp
Re: [PATCH 11/17] clk: imx: Add blk_ctrl combo driver
Hi Abel,
On Wed, 2020-07-29 at 15:07 +0300, Abel Vesa wrote:
> On i.MX8MP, there is a new type of IP which is called BLK_CTRL in
> RM and usually is comprised of some GPRs that are considered too
> generic to be part of any dedicated IP from that specific subsystem.
>
> In general, some of the GPRs have some clock bits, some have reset bits,
> so in order to be able to use the imx clock API, this needs to be
> in a clock driver. From there it can use the reset controller API and
> leave the rest to the syscon.
>
> This driver is intended to work with the following BLK_CTRL IPs found in
> i.MX8MP (but it might be reused by the future i.MX platforms that
> have this kind of IP in their design):
> - Audio
> - Media
> - HDMI
>
> Signed-off-by: Abel Vesa
> ---
> drivers/clk/imx/Makefile | 2 +-
> drivers/clk/imx/clk-blk-ctrl.c | 318
> +
> drivers/clk/imx/clk-blk-ctrl.h | 81 +++
> 3 files changed, 400 insertions(+), 1 deletion(-)
> create mode 100644 drivers/clk/imx/clk-blk-ctrl.c
> create mode 100644 drivers/clk/imx/clk-blk-ctrl.h
>
> diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
> index 928f874c..7afe1df 100644
> --- a/drivers/clk/imx/Makefile
> +++ b/drivers/clk/imx/Makefile
> @@ -27,7 +27,7 @@ obj-$(CONFIG_MXC_CLK_SCU) += \
>
> obj-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o
> obj-$(CONFIG_CLK_IMX8MN) += clk-imx8mn.o
> -obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o
> +obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o clk-blk-ctrl.o
> obj-$(CONFIG_CLK_IMX8MQ) += clk-imx8mq.o
> obj-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o
>
> diff --git a/drivers/clk/imx/clk-blk-ctrl.c b/drivers/clk/imx/clk-blk-ctrl.c
> new file mode 100644
> index ..a46e674
> --- /dev/null
> +++ b/drivers/clk/imx/clk-blk-ctrl.c
> @@ -0,0 +1,318 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2020 NXP.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "clk.h"
> +#include "clk-blk-ctrl.h"
> +
> +struct reset_hw {
> + u32 offset;
> + u32 shift;
> + u32 mask;
> +};
> +
> +struct pm_safekeep_info {
> + uint32_t *regs_values;
> + uint32_t *regs_offsets;
> + uint32_t regs_num;
> +};
> +
> +struct imx_blk_ctrl_drvdata {
> + void __iomem *base;
> + struct reset_controller_dev rcdev;
> + struct reset_hw *rst_hws;
> + struct pm_safekeep_info pm_info;
> +
> + spinlock_t lock;
> +};
> +
> +static int imx_blk_ctrl_reset_set(struct reset_controller_dev *rcdev,
> + unsigned long id, bool assert)
> +{
> + struct imx_blk_ctrl_drvdata *drvdata = container_of(rcdev,
> + struct imx_blk_ctrl_drvdata, rcdev);
> + unsigned int offset = drvdata->rst_hws[id].offset;
> + unsigned int shift = drvdata->rst_hws[id].shift;
> + unsigned int mask = drvdata->rst_hws[id].mask;
> + void __iomem *reg_addr = drvdata->base + offset;
> + unsigned long flags;
> + u32 reg;
> +
> + if (assert) {
> + pm_runtime_get_sync(rcdev->dev);
> + spin_lock_irqsave(>lock, flags);
> + reg = readl(reg_addr);
> + writel(reg & ~(mask << shift), reg_addr);
> + spin_unlock_irqrestore(>lock, flags);
> + } else {
> + spin_lock_irqsave(>lock, flags);
> + reg = readl(reg_addr);
> + writel(reg | (mask << shift), reg_addr);
> + spin_unlock_irqrestore(>lock, flags);
> + pm_runtime_put(rcdev->dev);
This still has the issue of potentially letting exclusive reset control
users break the device usage counter.
Also shared reset control users start with deassert(), and you end probe
with pm_runtime_put(), so the first shared reset control user that
deasserts its reset will decrement the dev->power.usage_count to -1 ?
For multiple resets being initially deasserted this would decrement
multiple times.
I think you'll have to track the (number of) asserted reset bits in this
reset controller and limit when to call pm_runtime_get/put_sync().
> + }
> +
> + return 0;
> +}
> +
> +static int imx_blk_ctrl_reset_reset(struct reset_controller_dev *rcdev,
> +unsigned long id)
> +{
> + imx_blk_ctrl_reset_set(rcdev, id, true);
> + return imx_blk_ctrl_reset_set(rcdev, id, false);
Does this work for all peripherals? Are there none that require the
reset line to be asserted for a certain number of bus clocks or similar?
> +}
> +
> +static int imx_blk_ctrl_reset_assert(struct reset_controller_dev *rcdev,
> +unsigned long id)
> +{
> + return imx_blk_ctrl_reset_set(rcdev, id, true);
> +}
> +
> +static int imx_blk_ctrl_reset_deassert(struct reset_controller_dev *rcdev,
> + unsigned
Re: [PATCH 1/2] reset-controller: ti: adjust the reset assert and deassert interface
Hi Crystal, Matthias,
On Wed, 2020-07-29 at 09:48 +0200, Matthias Brugger wrote:
>
> On 29/07/2020 09:39, Crystal Guo wrote:
> > Add ti_syscon_reset() to integrate assert and deassert together,
> > and change return value of the reset assert and deassert interface
> > from regmap_update_bits to regmap_write_bits.
> >
> > when clear bit is already 1, regmap_update_bits can not write 1 to it again.
> > Some IC has the feature that, when set bit is 1, the clear bit change
> > to 1 together. It will truly clear bit to 0 by write 1 to the clear bit
> >
> > Signed-off-by: Crystal Guo
> > ---
> > drivers/reset/reset-ti-syscon.c | 13 +++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/reset/reset-ti-syscon.c
> > b/drivers/reset/reset-ti-syscon.c
> > index a2635c2..5a8ec8f 100644
> > --- a/drivers/reset/reset-ti-syscon.c
> > +++ b/drivers/reset/reset-ti-syscon.c
> > @@ -89,7 +89,7 @@ static int ti_syscon_reset_assert(struct
> > reset_controller_dev *rcdev,
> > mask = BIT(control->assert_bit);
> > value = (control->flags & ASSERT_SET) ? mask : 0x0;
> >
> > - return regmap_update_bits(data->regmap, control->assert_offset, mask,
> > value);
> > + return regmap_write_bits(data->regmap, control->assert_offset, mask,
> > value);
>
> Nack, this will break the driver for the other devices.
I don't think this will break the driver for existing hardware.
regmap_write_bits() is the same as regmap_update_bits(), it just forces
the write in case the read already happens to return the correct value.
Of course it would be good to check that this actually works.
> The kernel has to work not just for your SoC but for all devices of all
> architectures. You can't just hack something up, that will work on your
> specific
> SoC.
>
> Regards,
> Matthias
>
> > }
> >
> > /**
> > @@ -120,7 +120,7 @@ static int ti_syscon_reset_deassert(struct
> > reset_controller_dev *rcdev,
> > mask = BIT(control->deassert_bit);
> > value = (control->flags & DEASSERT_SET) ? mask : 0x0;
> >
> > - return regmap_update_bits(data->regmap, control->deassert_offset, mask,
> > value);
> > + return regmap_write_bits(data->regmap, control->deassert_offset, mask,
> > value);
> > }
> >
> > /**
> > @@ -158,10 +158,19 @@ static int ti_syscon_reset_status(struct
> > reset_controller_dev *rcdev,
> > !(control->flags & STATUS_SET);
> > }
> >
> > +static int ti_syscon_reset(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + ti_syscon_reset_assert(rcdev, id);
> > +
> > + return ti_syscon_reset_deassert(rcdev, id);
> > +}
> > +
I'm unsure about this one, though. This is an incompatible change. At
the very least this would have to be optional depending on compatible.
regards
Philipp
Re: [PATCH] reset: Fix and extend kerneldoc
Hi Krzysztof, On Tue, 2020-07-28 at 19:10 +0200, Krzysztof Kozlowski wrote: > Fix W=1 compile warnings (invalid kerneldoc): > > drivers/reset/core.c:50: warning: Function parameter or member 'array' > not described in 'reset_control' > drivers/reset/core.c:50: warning: Function parameter or member > 'deassert_count' not described in 'reset_control' > > Signed-off-by: Krzysztof Kozlowski > --- > drivers/reset/core.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/reset/core.c b/drivers/reset/core.c > index 01c0c7aa835c..a2df88e90011 100644 > --- a/drivers/reset/core.c > +++ b/drivers/reset/core.c > @@ -32,7 +32,8 @@ static LIST_HEAD(reset_lookup_list); > * @refcnt: Number of gets of this reset_control > * @acquired: Only one reset_control may be acquired for a given rcdev and > id. > * @shared: Is this a shared (1), or an exclusive (0) reset_control? > - * @deassert_cnt: Number of times this reset line has been deasserted > + * @array: Is this an array of reset controls (1)? > + * @deassert_count: Number of times this reset line has been deasserted > * @triggered_count: Number of times this reset line has been reset. > Currently > * only used for shared resets, which means that the value > * will be either 0 or 1. Thank you for the fix, applied to reset/next. regards Philipp
Re: [PATCH v9 02/12] ata: ahci_brcm: Fix use of BCM7216 reset controller
Hi Jim,
On Fri, 2020-07-24 at 16:33 -0400, Jim Quinlan wrote:
> From: Jim Quinlan
>
> A reset controller "rescal" is shared between the AHCI driver and the PCIe
> driver for the BrcmSTB 7216 chip. Use
> devm_reset_control_get_optional_shared() to handle this sharing.
>
> Signed-off-by: Jim Quinlan
>
> Fixes: 272ecd60a636 ("ata: ahci_brcm: BCM7216 reset is self de-asserting")
> Fixes: c345ec6a50e9 ("ata: ahci_brcm: Support BCM7216 reset controller name")
> ---
> drivers/ata/ahci_brcm.c | 11 +++
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
> index 6853dbb4131d..d6115bc04b09 100644
> --- a/drivers/ata/ahci_brcm.c
> +++ b/drivers/ata/ahci_brcm.c
> @@ -428,7 +428,6 @@ static int brcm_ahci_probe(struct platform_device *pdev)
> {
> const struct of_device_id *of_id;
> struct device *dev = >dev;
> - const char *reset_name = NULL;
> struct brcm_ahci_priv *priv;
> struct ahci_host_priv *hpriv;
> struct resource *res;
> @@ -452,11 +451,10 @@ static int brcm_ahci_probe(struct platform_device *pdev)
>
> /* Reset is optional depending on platform and named differently */
> if (priv->version == BRCM_SATA_BCM7216)
> - reset_name = "rescal";
> + priv->rcdev =
> devm_reset_control_get_optional_shared(>dev, "rescal");
> else
> - reset_name = "ahci";
> + priv->rcdev = devm_reset_control_get_optional(>dev,
> "ahci");
Please use devm_reset_control_get_optional_exclusive() here.
>
> - priv->rcdev = devm_reset_control_get_optional(>dev, reset_name);
> if (IS_ERR(priv->rcdev))
> return PTR_ERR(priv->rcdev);
>
> @@ -479,10 +477,7 @@ static int brcm_ahci_probe(struct platform_device *pdev)
> break;
> }
>
> - if (priv->version == BRCM_SATA_BCM7216)
> - ret = reset_control_reset(priv->rcdev);
> - else
> - ret = reset_control_deassert(priv->rcdev);
> + ret = reset_control_deassert(priv->rcdev);
> if (ret)
> return ret;
>
Since you switch from _reset to _deassert/_assert here, I suspect you
should also change the out_reset error path, and the suspend/resume
functions accordingly.
regards
Philipp
Re: [PATCH V3 1/3] reset: imx7: Support module build
On Mon, 2020-07-20 at 22:21 +0800, Anson Huang wrote: > Use module_platform_driver(), add module device table, author, > description and license to support module build, and > CONFIG_RESET_IMX7 is changed to default 'y' ONLY for i.MX7D, > other platforms need to select it in defconfig. > > Signed-off-by: Anson Huang > --- > Changes since V2: > - use module_platform_driver() instead of builtin_platform_driver(). Thank you, applied to reset/next. regards Philipp
Re: [PATCH v2 0/2] reset: reset-zynqmp: Added Versal platform support
On Wed, 2020-07-22 at 12:46 +0530, Sai Krishna Potthuri wrote: > Extended the ZynqMP reset driver to support Versal platform, accordingly > updated the dt-binding with the Versal platform specific information > like compatible string and reset indices. > > Changes in v2: > - Updated 1/2 patch commit description with some explanation about > reset indices. > - 2/2 patch, Created SOC specific data structure and initialize the > const data based on of_device_get_match_data. > - Defined driver specific of_xlate callback. > > Sai Krishna Potthuri (2): > dt-bindings: reset: Updated binding for Versal reset driver > reset: reset-zynqmp: Added support for Versal platform > > .../bindings/reset/xlnx,zynqmp-reset.txt | 11 +- > drivers/reset/reset-zynqmp.c | 50 - > .../dt-bindings/reset/xlnx-versal-resets.h| 105 ++ > 3 files changed, 156 insertions(+), 10 deletions(-) > create mode 100644 include/dt-bindings/reset/xlnx-versal-resets.h Thank you, applied to reset/next. regards Philipp
Re: [PATCH v7 2/5] drm/imx: Add initial support for DCSS on iMX8MQ
Hi Laurentiu, On Tue, 2020-07-21 at 13:20 +0300, Laurentiu Palcu wrote: > From: Laurentiu Palcu > > This adds initial support for iMX8MQ's Display Controller Subsystem (DCSS). > Some of its capabilities include: > * 4K@60fps; > * HDR10; > * one graphics and 2 video pipelines; > * on-the-fly decompression of compressed video and graphics; > > The reference manual can be found here: > https://www.nxp.com/webapp/Download?colCode=IMX8MDQLQRM > > The current patch adds only basic functionality: one primary plane for > graphics, linear, tiled and super-tiled buffers support (no graphics > decompression yet), no HDR10 and no video planes. > > Video planes support and HDR10 will be added in subsequent patches once > per-plane de-gamma/CSC/gamma support is in. > > Signed-off-by: Laurentiu Palcu > Reviewed-by: Lucas Stach > --- > drivers/gpu/drm/imx/Kconfig| 2 + > drivers/gpu/drm/imx/Makefile | 1 + > drivers/gpu/drm/imx/dcss/Kconfig | 9 + > drivers/gpu/drm/imx/dcss/Makefile | 6 + > drivers/gpu/drm/imx/dcss/dcss-blkctl.c | 70 +++ > drivers/gpu/drm/imx/dcss/dcss-crtc.c | 219 +++ > drivers/gpu/drm/imx/dcss/dcss-ctxld.c | 424 + > drivers/gpu/drm/imx/dcss/dcss-dev.c| 314 ++ > drivers/gpu/drm/imx/dcss/dcss-dev.h| 177 ++ > drivers/gpu/drm/imx/dcss/dcss-dpr.c| 562 + > drivers/gpu/drm/imx/dcss/dcss-drv.c| 138 + > drivers/gpu/drm/imx/dcss/dcss-dtg.c| 409 > drivers/gpu/drm/imx/dcss/dcss-kms.c| 177 ++ > drivers/gpu/drm/imx/dcss/dcss-kms.h| 43 ++ > drivers/gpu/drm/imx/dcss/dcss-plane.c | 405 > drivers/gpu/drm/imx/dcss/dcss-scaler.c | 826 + > drivers/gpu/drm/imx/dcss/dcss-ss.c | 180 ++ > 17 files changed, 3962 insertions(+) > create mode 100644 drivers/gpu/drm/imx/dcss/Kconfig > create mode 100644 drivers/gpu/drm/imx/dcss/Makefile > create mode 100644 drivers/gpu/drm/imx/dcss/dcss-blkctl.c > create mode 100644 drivers/gpu/drm/imx/dcss/dcss-crtc.c > create mode 100644 drivers/gpu/drm/imx/dcss/dcss-ctxld.c > create mode 100644 drivers/gpu/drm/imx/dcss/dcss-dev.c > create mode 100644 drivers/gpu/drm/imx/dcss/dcss-dev.h > create mode 100644 drivers/gpu/drm/imx/dcss/dcss-dpr.c > create mode 100644 drivers/gpu/drm/imx/dcss/dcss-drv.c > create mode 100644 drivers/gpu/drm/imx/dcss/dcss-dtg.c > create mode 100644 drivers/gpu/drm/imx/dcss/dcss-kms.c > create mode 100644 drivers/gpu/drm/imx/dcss/dcss-kms.h > create mode 100644 drivers/gpu/drm/imx/dcss/dcss-plane.c > create mode 100644 drivers/gpu/drm/imx/dcss/dcss-scaler.c > create mode 100644 drivers/gpu/drm/imx/dcss/dcss-ss.c > > diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig > index 207bf7409dfb..6231048aa5aa 100644 > --- a/drivers/gpu/drm/imx/Kconfig > +++ b/drivers/gpu/drm/imx/Kconfig > @@ -39,3 +39,5 @@ config DRM_IMX_HDMI > depends on DRM_IMX > help > Choose this if you want to use HDMI on i.MX6. > + > +source "drivers/gpu/drm/imx/dcss/Kconfig" > diff --git a/drivers/gpu/drm/imx/Makefile b/drivers/gpu/drm/imx/Makefile > index 21cdcc2faabc..b644deffe948 100644 > --- a/drivers/gpu/drm/imx/Makefile > +++ b/drivers/gpu/drm/imx/Makefile > @@ -9,3 +9,4 @@ obj-$(CONFIG_DRM_IMX_TVE) += imx-tve.o > obj-$(CONFIG_DRM_IMX_LDB) += imx-ldb.o > > obj-$(CONFIG_DRM_IMX_HDMI) += dw_hdmi-imx.o > +obj-$(CONFIG_DRM_IMX_DCSS) += dcss/ > diff --git a/drivers/gpu/drm/imx/dcss/Kconfig > b/drivers/gpu/drm/imx/dcss/Kconfig > new file mode 100644 > index ..988979bc22cc > --- /dev/null > +++ b/drivers/gpu/drm/imx/dcss/Kconfig > @@ -0,0 +1,9 @@ > +config DRM_IMX_DCSS > + tristate "i.MX8MQ DCSS" > + select RESET_CONTROLLER Why does DCSS select RESET_CONTROLLER? regards Philipp
Re: [PATCH] drm/imx: imx-tve: Delete an error message in imx_tve_bind()
On Sun, 2020-04-05 at 11:16 +0200, Markus Elfring wrote: > From: Markus Elfring > Date: Sun, 5 Apr 2020 11:01:49 +0200 > > The function “platform_get_irq” can log an error already. > Thus omit a redundant message for the exception handling in the > calling function. > > This issue was detected by using the Coccinelle software. > > Signed-off-by: Markus Elfring Thank you, applied to imx-drm/next. regards Philipp
Re: [PATCH] reset: Replace HTTP links with HTTPS ones
On Sat, 2020-07-18 at 14:18 +0200, Alexander A. Klimov wrote: > Rationale: > Reduces attack surface on kernel devs opening the links for MITM > as HTTPS traffic is much harder to manipulate. > > Deterministic algorithm: > For each file: > If not .svg: > For each line: > If doesn't contain `\bxmlns\b`: > For each link, `\bhttp://[^# \t\r\n]*(?:\w|/)`: > If neither `\bgnu\.org/license`, nor `\bmozilla\.org/MPL\b`: > If both the HTTP and HTTPS versions > return 200 OK and serve the same content: > Replace HTTP with HTTPS. > > Signed-off-by: Alexander A. Klimov > --- > Continuing my work started at 93431e0607e5. > See also: git log --oneline '--author=Alexander A. Klimov > ' v5.7..master > (Actually letting a shell for loop submit all this stuff for me.) > > If there are any URLs to be removed completely > or at least not (just) HTTPSified: > Just clearly say so and I'll *undo my change*. > See also: https://lkml.org/lkml/2020/6/27/64 > > If there are any valid, but yet not changed URLs: > See: https://lkml.org/lkml/2020/6/26/837 > > If you apply the patch, please let me know. Thank you, applied to reset/next. regards Philipp
Re: [PATCH 2/2] reset: reset-zynqmp: Added support for Versal platform
On Tue, 2020-07-14 at 11:59 +0530, Sai Krishna Potthuri wrote:
> Updated the reset driver to support Versal platform.
> As part of adding Versal support
> - Added Versal specific compatible string.
> - Reset Id and number of resets are different for Versal and ZynqMP,
> hence taken care of these two based on compatible string.
>
> Signed-off-by: Sai Krishna Potthuri
> ---
> drivers/reset/reset-zynqmp.c | 24
> 1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/reset/reset-zynqmp.c b/drivers/reset/reset-zynqmp.c
> index 373ea8d4f7a1..17aa4532ec5e 100644
> --- a/drivers/reset/reset-zynqmp.c
> +++ b/drivers/reset/reset-zynqmp.c
> @@ -12,9 +12,11 @@
>
> #define ZYNQMP_NR_RESETS (ZYNQMP_PM_RESET_END - ZYNQMP_PM_RESET_START)
> #define ZYNQMP_RESET_ID ZYNQMP_PM_RESET_START
> +#define VERSAL_NR_RESETS 95
>
> struct zynqmp_reset_data {
> struct reset_controller_dev rcdev;
> + u32 reset_id;
> };
>
> static inline struct zynqmp_reset_data *
> @@ -26,23 +28,28 @@ to_zynqmp_reset_data(struct reset_controller_dev *rcdev)
> static int zynqmp_reset_assert(struct reset_controller_dev *rcdev,
> unsigned long id)
> {
> - return zynqmp_pm_reset_assert(ZYNQMP_RESET_ID + id,
> + struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
> +
> + return zynqmp_pm_reset_assert(priv->reset_id + id,
> PM_RESET_ACTION_ASSERT);
> }
>
> static int zynqmp_reset_deassert(struct reset_controller_dev *rcdev,
>unsigned long id)
> {
> - return zynqmp_pm_reset_assert(ZYNQMP_RESET_ID + id,
> + struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
> +
> + return zynqmp_pm_reset_assert(priv->reset_id + id,
> PM_RESET_ACTION_RELEASE);
> }
>
> static int zynqmp_reset_status(struct reset_controller_dev *rcdev,
> unsigned long id)
> {
> + struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
> int val, err;
>
> - err = zynqmp_pm_reset_get_status(ZYNQMP_RESET_ID + id, );
> + err = zynqmp_pm_reset_get_status(priv->reset_id + id, );
> if (err)
> return err;
>
> @@ -52,7 +59,9 @@ static int zynqmp_reset_status(struct reset_controller_dev
> *rcdev,
> static int zynqmp_reset_reset(struct reset_controller_dev *rcdev,
> unsigned long id)
> {
> - return zynqmp_pm_reset_assert(ZYNQMP_RESET_ID + id,
> + struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
> +
> + return zynqmp_pm_reset_assert(priv->reset_id + id,
> PM_RESET_ACTION_PULSE);
> }
>
> @@ -76,13 +85,20 @@ static int zynqmp_reset_probe(struct platform_device
> *pdev)
> priv->rcdev.ops = _reset_ops;
> priv->rcdev.owner = THIS_MODULE;
> priv->rcdev.of_node = pdev->dev.of_node;
> + priv->reset_id = ZYNQMP_RESET_ID;
> priv->rcdev.nr_resets = ZYNQMP_NR_RESETS;
> + if (of_device_is_compatible(pdev->dev.of_node,
> + "xlnx,versal-reset")) {
It would be better to use of_match_device and static const initalization
data for this.
> + priv->reset_id = 0;
> + priv->rcdev.nr_resets = VERSAL_NR_RESETS;
This won't work. All your reset ids are greater than 95, and this driver
is using the default of_xlate callback, so of_reset_simple_xlate will
fail all reset control requests with -EINVAL.
regards
Philipp
Re: [Patch v1 2/4] dma: tegra: Adding Tegra GPC DMA controller driver
Hi Rajesh,
On Mon, 2020-07-20 at 12:04 +0530, Rajesh Gumasta wrote:
> +
> + tdma->rst = devm_reset_control_get(>dev, "gpcdma");
Please use devm_reset_control_get_exclusive() directly.
> + if (IS_ERR(tdma->rst)) {
> + dev_err(>dev, "Missing controller reset\n");
You might want to suppress this message if the error is EPROBE_DEFER.
> + return PTR_ERR(tdma->rst);
> + }
> + reset_control_reset(tdma->rst);
regards
Philipp
Re: [PATCH V2 1/3] reset: imx7: Support module build
On Mon, 2020-06-29 at 23:05 +0800, Anson Huang wrote:
> Add module device table, author, description and license to support
> module build, and CONFIG_RESET_IMX7 is changed to default 'y' ONLY
> for i.MX7D, other platforms need to select it in defconfig.
>
> Signed-off-by: Anson Huang
> ---
> Changes since V1:
> - make it default 'y' for SOC_IMX7D;
> - add module author, description;
> - use device_initcall instead of builtin_platform_driver() to support
> module unload.
> ---
> drivers/reset/Kconfig | 5 +++--
> drivers/reset/reset-imx7.c | 14 --
> 2 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index d9efbfd..19f9773 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -65,9 +65,10 @@ config RESET_HSDK
> This enables the reset controller driver for HSDK board.
>
> config RESET_IMX7
> - bool "i.MX7/8 Reset Driver" if COMPILE_TEST
> + tristate "i.MX7/8 Reset Driver"
> depends on HAS_IOMEM
> - default SOC_IMX7D || (ARM64 && ARCH_MXC)
> + depends on SOC_IMX7D || (ARM64 && ARCH_MXC) || COMPILE_TEST
> + default y if SOC_IMX7D
> select MFD_SYSCON
> help
> This enables the reset controller driver for i.MX7 SoCs.
> diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> index d170fe6..c710f789 100644
> --- a/drivers/reset/reset-imx7.c
> +++ b/drivers/reset/reset-imx7.c
> @@ -8,7 +8,7 @@
> */
>
> #include
> -#include
> +#include
> #include
> #include
> #include
> @@ -386,6 +386,7 @@ static const struct of_device_id imx7_reset_dt_ids[] = {
> { .compatible = "fsl,imx8mp-src", .data = _imx8mp },
> { /* sentinel */ },
> };
> +MODULE_DEVICE_TABLE(of, imx7_reset_dt_ids);
>
> static struct platform_driver imx7_reset_driver = {
> .probe = imx7_reset_probe,
> @@ -394,4 +395,13 @@ static struct platform_driver imx7_reset_driver = {
> .of_match_table = imx7_reset_dt_ids,
> },
> };
> -builtin_platform_driver(imx7_reset_driver);
> +
> +static int __init imx7_reset_init(void)
> +{
> + return platform_driver_register(_reset_driver);
> +}
> +device_initcall(imx7_reset_init);
Shouldn't this use module_platform_driver instead?
regards
Philipp
Re: [PATCH] drm: imx: Fix occasional screen corruption on modeset.
Hi Martin,
On Tue, 2020-07-07 at 17:56 +0200, Martin Fuzzey wrote:
> When performing a modeset the atomic core calls
> ipu_crtc_atomic_disable() which switches off the DC and DI.
>
> When we immediately restart as in the modeset case this sometimes
> leads to corruption at the bottom of the screen looking like a mirror
> image of the top.
Could this be just a panel getting confused because the pixel clock is
disabled, or is there really an issue with the IPU? Have you tried just
keeping clk_di_pixel enabled in ipu_di_disable(), but continuing
to disable DI and DC?
> The exact reason isn't understood but it seems timing related.
>
> This was observed on i.MX6DL on a system that does 2 mode
> transitions on boot (fbcon->android boot animation->android homescreen)
> then no more during normal operation resulting in corruption
> about once every 10 boots that lasted for variable durations
> from a few seconds to several hours.
>
> Dumping the buffers confirmed that they were correct in memory,
> just the display was wrong.
>
> For tests the problem was reproduced systematically by forcing
> a full modeset every 10 frames even if when not required.
>
> Leaving the DC and DI on if the CRTC is staying on fixes this.
>
> Signed-off-by: Martin Fuzzey
> ---
> drivers/gpu/drm/imx/ipuv3-crtc.c | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c
> b/drivers/gpu/drm/imx/ipuv3-crtc.c
> index 63c0284..9137b64 100644
> --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> @@ -84,8 +84,15 @@ static void ipu_crtc_atomic_disable(struct drm_crtc *crtc,
> struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
> struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent);
>
> - ipu_dc_disable_channel(ipu_crtc->dc);
> - ipu_di_disable(ipu_crtc->di);
> + /*
> + * If we are just doing a modeset don't disable dc/di as that
> + * sometimes leads to corruption at the bottom of the screen
> + */
> + if (!crtc->state->active) {
> + ipu_dc_disable_channel(ipu_crtc->dc);
> + ipu_di_disable(ipu_crtc->di);
Just removing ipu_di_disable() leaks a clk_prepare_enable refcount on
the di->clk_di_pixel clock.
Also this is followed by an ipu_dc_disable(), which should remove the DC
module's clock if this is the only display. So the DC should be disabled
anyway.
> + }
> +
> /*
>* Planes must be disabled before DC clock is removed, as otherwise the
>* attached IDMACs will be left in undefined state, possibly hanging
regards
Philipp
Re: [PATCH v2 0/6] Hantro low-hanging cleanups
On Wed, 2020-07-01 at 10:16 -0300, Ezequiel Garcia wrote: > Second iteration, just addressing Philipp's and Robin's > feedback on patch 3. Thank you, feel free to add Reviewed-by: Philipp Zabel for the series. regards Philipp
Re: [PATCH 2/2] hantro: h264: Refuse to decode unsupported bitstream
On Fri, 2020-06-26 at 14:11 -0300, Ezequiel Garcia wrote: > The hardware only supports 4:2:0 or 4:0:0 (monochrome), > 8-bit depth content. > > Verify that the PPS refers to a supported bitstream, and refuse > unsupported bitstreams by failing at TRY_EXT_CTRLS time. > > Given the JPEG compression level control is the only one > that needs setting, a specific ops is provided. > > Signed-off-by: Ezequiel Garcia Reviewed-by: Philipp Zabel regards Philipp
Re: [PATCH v2 4/6] dt-bindings: reset: Add binding constants for Actions S500 RMU
On Wed, 2020-06-24 at 20:47 +0300, Cristian Ciocaltea wrote: > Add device tree binding constants for Actions Semi S500 SoC Reset > Management Unit (RMU). > > Signed-off-by: Cristian Ciocaltea Acked-by: Philipp Zabel to be merged through the clock tree, required by the following patch: "clk: actions: Add Actions S500 SoC Reset Management Unit support". regards Philipp > --- > .../dt-bindings/reset/actions,s500-reset.h| 67 +++ > 1 file changed, 67 insertions(+) > create mode 100644 include/dt-bindings/reset/actions,s500-reset.h > > diff --git a/include/dt-bindings/reset/actions,s500-reset.h > b/include/dt-bindings/reset/actions,s500-reset.h > new file mode 100644 > index ..f5d94176d10b > --- /dev/null > +++ b/include/dt-bindings/reset/actions,s500-reset.h > @@ -0,0 +1,67 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ > +/* > + * Device Tree binding constants for Actions Semi S500 Reset Management Unit > + * > + * Copyright (c) 2014 Actions Semi Inc. > + * Copyright (c) 2020 Cristian Ciocaltea > + */ > + > +#ifndef __DT_BINDINGS_ACTIONS_S500_RESET_H > +#define __DT_BINDINGS_ACTIONS_S500_RESET_H > + > +#define RESET_DMAC 0 > +#define RESET_NORIF 1 > +#define RESET_DDR2 > +#define RESET_NANDC 3 > +#define RESET_SD04 > +#define RESET_SD15 > +#define RESET_PCM1 6 > +#define RESET_DE 7 > +#define RESET_LCD8 > +#define RESET_SD29 > +#define RESET_DSI10 > +#define RESET_CSI11 > +#define RESET_BISP 12 > +#define RESET_KEY13 > +#define RESET_GPIO 14 > +#define RESET_AUDIO 15 > +#define RESET_PCM0 16 > +#define RESET_VDE17 > +#define RESET_VCE18 > +#define RESET_GPU3D 19 > +#define RESET_NIC301 20 > +#define RESET_LENS 21 > +#define RESET_PERIPHRESET22 > +#define RESET_USB2_0 23 > +#define RESET_TVOUT 24 > +#define RESET_HDMI 25 > +#define RESET_HDCP2TX26 > +#define RESET_UART6 27 > +#define RESET_UART0 28 > +#define RESET_UART1 29 > +#define RESET_UART2 30 > +#define RESET_SPI0 31 > +#define RESET_SPI1 32 > +#define RESET_SPI2 33 > +#define RESET_SPI3 34 > +#define RESET_I2C0 35 > +#define RESET_I2C1 36 > +#define RESET_USB3 37 > +#define RESET_UART3 38 > +#define RESET_UART4 39 > +#define RESET_UART5 40 > +#define RESET_I2C2 41 > +#define RESET_I2C3 42 > +#define RESET_ETHERNET 43 > +#define RESET_CHIPID 44 > +#define RESET_USB2_1 45 > +#define RESET_WD0RESET 46 > +#define RESET_WD1RESET 47 > +#define RESET_WD2RESET 48 > +#define RESET_WD3RESET 49 > +#define RESET_DBG0RESET 50 > +#define RESET_DBG1RESET 51 > +#define RESET_DBG2RESET 52 > +#define RESET_DBG3RESET 53 > + > +#endif /* __DT_BINDINGS_ACTIONS_S500_RESET_H */
Re: [PATCH v3 2/9] reset: Add Raspberry Pi 4 firmware reset controller
On Wed, 2020-06-17 at 12:44 +0200, Nicolas Saenz Julienne wrote: > On Wed, 2020-06-17 at 12:02 +0200, Philipp Zabel wrote: > > Hi Nicolas, > > > > On Fri, 2020-06-12 at 19:13 +0200, Nicolas Saenz Julienne wrote: > > > Raspberry Pi 4's co-processor controls some of the board's HW > > > initialization process, but it's up to Linux to trigger it when > > > relevant. Introduce a reset controller capable of interfacing with > > > RPi4's co-processor that models these firmware initialization routines as > > > reset lines. > > > > > > Signed-off-by: Nicolas Saenz Julienne > > > Reviewed-by: Florian Fainelli > > > > Reviewed-by: Philipp Zabel > > Thanks! > > > If there is a good reason for the single DT specified reset id, I can > > pick up patches 1 and 2. > > The idea here is to make sure we're reasonably covered against further changes > in firmware. If we define constraints too narrow it can be a pain to support > new features without breaking backwards compatibility in dt. Ok. > > If you change the dts patch 4 to use a number instead of the reset id > > define for now, there wouldn't even be a dependency between these reset > > and dts patches. > > I was under the impression that having an explicit definition was nice to > have. > What's troubling about creating the dependency? Just that the last patch has to wait for the reset patches to be merged before it can be applied. regards Philipp
Re: [PATCH v1] reset: intel: fix a compile warning about REG_OFFSET redefined
Hi Dejin,
On Thu, 2020-06-04 at 23:30 +0800, Dejin Zheng wrote:
> kernel test robot reports a compile warning about REG_OFFSET redefined
> in the reset-intel-gw.c after merging commit e44ab4e14d6f4 ("regmap:
> Simplify implementation of the regmap_read_poll_timeout() macro"). the
> warning is like that:
>
> drivers/reset/reset-intel-gw.c:18:0: warning: "REG_OFFSET" redefined
> #define REG_OFFSET GENMASK(31, 16)
>
> In file included from ./arch/arm/mach-ixp4xx/include/mach/hardware.h:30:0,
> from ./arch/arm/mach-ixp4xx/include/mach/io.h:15,
> from ./arch/arm/include/asm/io.h:198,
> from ./include/linux/io.h:13,
> from ./include/linux/iopoll.h:14,
> from ./include/linux/regmap.h:20,
> from drivers/reset/reset-intel-gw.c:12:
> ./arch/arm/mach-ixp4xx/include/mach/platform.h:25:0: note: this is the
> location of the previous definition
> #define REG_OFFSET 3
>
> Reported-by: kernel test robot
> Fixes: e44ab4e14d6f4 ("regmap: Simplify implementation of the
> regmap_read_poll_timeout() macro")
Hm, shouldn't this rather be:
Fixes: c9aef213e38c ("reset: intel: Add system reset controller driver")
even though e44ab4e14d6f4 triggered the issue by including iopoll.h?
> Signed-off-by: Dejin Zheng
> ---
> drivers/reset/reset-intel-gw.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/reset/reset-intel-gw.c b/drivers/reset/reset-intel-gw.c
> index 854238444616..5cfb4892b399 100644
> --- a/drivers/reset/reset-intel-gw.c
> +++ b/drivers/reset/reset-intel-gw.c
> @@ -15,7 +15,7 @@
> #define RCU_RST_STAT 0x0024
> #define RCU_RST_REQ 0x0048
>
> -#define REG_OFFSET GENMASK(31, 16)
> +#define REG_OFFSET_MASK GENMASK(31, 16)
> #define BIT_OFFSET GENMASK(15, 8)
> #define STAT_BIT_OFFSET GENMASK(7, 0)
Could you add the _MASK suffix to BIT_OFFSET and STAT_BIT_OFFSET as
well, for consistency?
regards
Philipp
Re: [PATCH 3/6] hantro: Rework how encoder and decoder are identified
Hi Ezequiel,
On Thu, 2020-06-25 at 13:35 -0300, Ezequiel Garcia wrote:
> So far we've been using the .buf_finish hook to distinguish
> decoder from encoder. This is unnecessarily obfuscated.
>
> Moreover, we want to move the buf_finish, so use a cleaner
> scheme to distinguish the driver decoder/encoder type.
>
> Signed-off-by: Ezequiel Garcia
> ---
> drivers/staging/media/hantro/hantro.h | 2 ++
> drivers/staging/media/hantro/hantro_drv.c | 4 +++-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/hantro/hantro.h
> b/drivers/staging/media/hantro/hantro.h
> index 3005207fc6fb..028b788ad50f 100644
> --- a/drivers/staging/media/hantro/hantro.h
> +++ b/drivers/staging/media/hantro/hantro.h
> @@ -199,6 +199,7 @@ struct hantro_dev {
> *
> * @dev: VPU driver data to which the context belongs.
> * @fh: V4L2 file handler.
> + * @is_encoder: Decoder or encoder context?
> *
> * @sequence_cap: Sequence counter for capture queue
> * @sequence_out: Sequence counter for output queue
> @@ -223,6 +224,7 @@ struct hantro_dev {
> struct hantro_ctx {
> struct hantro_dev *dev;
> struct v4l2_fh fh;
> + bool is_encoder;
>
> u32 sequence_cap;
> u32 sequence_out;
> diff --git a/drivers/staging/media/hantro/hantro_drv.c
> b/drivers/staging/media/hantro/hantro_drv.c
> index 0db8ad455160..112ed556eb90 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -197,7 +197,7 @@ static void device_run(void *priv)
>
> bool hantro_is_encoder_ctx(const struct hantro_ctx *ctx)
> {
> - return ctx->buf_finish == hantro_enc_buf_finish;
> + return ctx->is_encoder;
> }
Now this function feels a little superfluous. Why not replace
hantro_is_encoder_ctx(ctx)
with
ctx->is_encoder
everywhere?
regards
Philipp
Re: [PATCH v4 2/2] phy: bcm63xx-usbh: Add BCM63xx USBH driver
Hi Álvaro,
On Fri, 2020-06-19 at 10:51 +0200, Álvaro Fernández Rojas wrote:
> Add BCM63xx USBH PHY driver for BMIPS.
>
> Signed-off-by: Álvaro Fernández Rojas
> ---
> v4: several improvements:
> - Use devm_platform_ioremap_resource.
> - Code cleanups.
> - Improve device mode config:
> - Move USBH_SWAP_CONTROL device mode value to variant variable.
> - Set USBH_UTMI_CONTROL1 register value (variant variable).
> v3: introduce changes suggested by Florian:
> - Add support for device mode.
> v2: introduce changes suggested by Florian:
> - Drop OF dependency (use device_get_match_data).
> - Drop __initconst from variant tables.
> - Use devm_clk_get_optional.
>
> drivers/phy/broadcom/Kconfig| 9 +
> drivers/phy/broadcom/Makefile | 1 +
> drivers/phy/broadcom/phy-bcm63xx-usbh.c | 457
> 3 files changed, 467 insertions(+)
> create mode 100644 drivers/phy/broadcom/phy-bcm63xx-usbh.c
>
[...]
> diff --git a/drivers/phy/broadcom/phy-bcm63xx-usbh.c
> b/drivers/phy/broadcom/phy-bcm63xx-usbh.c
> new file mode 100644
> index ..79f913d86def
> --- /dev/null
> +++ b/drivers/phy/broadcom/phy-bcm63xx-usbh.c
[...]
> +static int __init bcm63xx_usbh_phy_probe(struct platform_device *pdev)
> +{
> + struct device *dev = >dev;
> + struct bcm63xx_usbh_phy *usbh;
> + const struct bcm63xx_usbh_phy_variant *variant;
> + struct phy *phy;
> + struct phy_provider *phy_provider;
> +
> + usbh = devm_kzalloc(dev, sizeof(*usbh), GFP_KERNEL);
> + if (!usbh)
> + return -ENOMEM;
> +
> + variant = device_get_match_data(dev);
> + if (!variant)
> + return -EINVAL;
> + usbh->variant = variant;
> +
> + usbh->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(usbh->base))
> + return PTR_ERR(usbh->base);
> +
> + usbh->reset = devm_reset_control_get(dev, NULL);
Please use devm_reset_control_get_exclusive() instead.
regards
Philipp
Re: [PATCH v2 2/2] Add PWM fan controller driver for LGM SoC
Hi Rahul,
On Thu, 2020-06-18 at 20:05 +0800, Rahul Tanwar wrote:
> Intel Lightning Mountain(LGM) SoC contains a PWM fan controller.
> This PWM controller does not have any other consumer, it is a
> dedicated PWM controller for fan attached to the system. Add
> driver for this PWM fan controller.
>
> Signed-off-by: Rahul Tanwar
> ---
> drivers/pwm/Kconfig | 9 +
> drivers/pwm/Makefile| 1 +
> drivers/pwm/pwm-intel-lgm.c | 400
>
> 3 files changed, 410 insertions(+)
> create mode 100644 drivers/pwm/pwm-intel-lgm.c
>
[...]
> diff --git a/drivers/pwm/pwm-intel-lgm.c b/drivers/pwm/pwm-intel-lgm.c
> new file mode 100644
> index ..3c7077acb161
> --- /dev/null
> +++ b/drivers/pwm/pwm-intel-lgm.c
> @@ -0,0 +1,400 @@
[...]
> +static int lgm_pwm_probe(struct platform_device *pdev)
> +{
> + struct lgm_pwm_chip *pc;
> + struct device *dev = >dev;
> + void __iomem *io_base;
> + int ret;
> +
> + pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
> + if (!pc)
> + return -ENOMEM;
> +
> + io_base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(io_base))
> + return PTR_ERR(io_base);
> +
> + pc->regmap = devm_regmap_init_mmio(dev, io_base, _regmap_config);
> + if (IS_ERR(pc->regmap)) {
> + ret = PTR_ERR(pc->regmap);
> + dev_err(dev, "failed to init register map: %pe\n", pc->regmap);
> + return ret;
> + }
> +
> + pc->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(pc->clk)) {
> + ret = PTR_ERR(pc->clk);
> + dev_err(dev, "failed to get clock: %pe\n", pc->clk);
> + return ret;
> + }
> +
> + pc->rst = devm_reset_control_get(dev, NULL);
> + if (IS_ERR(pc->rst)) {
> + ret = PTR_ERR(pc->rst);
> + dev_err(dev, "failed to get reset control: %pe\n", pc->rst);
> + return ret;
> + }
Please use devm_reset_control_get_exclusive() to make it explicit an
that exclusive reset control is requested. Given how the reset control
is used, I think this driver could also use
devm_reset_control_get_shared() to potentially allow sharing a reset
line with other devices.
regards
Philipp
Re: [PATCH v3 5/9] usb: xhci-pci: Add support for reset controllers
On Fri, 2020-06-12 at 19:13 +0200, Nicolas Saenz Julienne wrote:
> Some atypical users of xhci-pci might need to manually reset their xHCI
> controller before starting the HCD setup. Check if a reset controller
> device is available to the PCI bus and trigger a reset.
>
> Signed-off-by: Nicolas Saenz Julienne
>
> ---
>
> Changes since v2:
> - Also reset on resume
>
> Changes since v1:
> - Use proper reset API
> - Make code simpler
>
> drivers/usb/host/xhci-pci.c | 10 ++
> drivers/usb/host/xhci.h | 2 ++
> 2 files changed, 12 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index ef513c2fb843..e76b9283faa3 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -12,6 +12,7 @@
> #include
> #include
> #include
> +#include
>
> #include "xhci.h"
> #include "xhci-trace.h"
> @@ -339,6 +340,7 @@ static int xhci_pci_probe(struct pci_dev *dev, const
> struct pci_device_id *id)
> struct xhci_hcd *xhci;
> struct usb_hcd *hcd;
> struct xhci_driver_data *driver_data;
> + struct reset_control *reset;
>
> driver_data = (struct xhci_driver_data *)id->driver_data;
> if (driver_data && driver_data->quirks & XHCI_RENESAS_FW_QUIRK) {
> @@ -347,6 +349,11 @@ static int xhci_pci_probe(struct pci_dev *dev, const
> struct pci_device_id *id)
> return retval;
> }
>
> + reset = devm_reset_control_get_optional_exclusive(>bus->dev, NULL);
> + if (IS_ERR(reset))
> + return PTR_ERR(reset);
> + reset_control_reset(reset);
Reviewed-by: Philipp Zabel
regards
Philipp
Re: [PATCH v3 2/9] reset: Add Raspberry Pi 4 firmware reset controller
Hi Nicolas, On Fri, 2020-06-12 at 19:13 +0200, Nicolas Saenz Julienne wrote: > Raspberry Pi 4's co-processor controls some of the board's HW > initialization process, but it's up to Linux to trigger it when > relevant. Introduce a reset controller capable of interfacing with > RPi4's co-processor that models these firmware initialization routines as > reset lines. > > Signed-off-by: Nicolas Saenz Julienne > Reviewed-by: Florian Fainelli Reviewed-by: Philipp Zabel If there is a good reason for the single DT specified reset id, I can pick up patches 1 and 2. If you change the dts patch 4 to use a number instead of the reset id define for now, there wouldn't even be a dependency between these reset and dts patches. regards Philipp
Re: [PATCH v3 1/9] dt-bindings: reset: Add a binding for the RPi Firmware reset controller
Hi Nicolas,
On Fri, 2020-06-12 at 19:13 +0200, Nicolas Saenz Julienne wrote:
> The firmware running on the RPi VideoCore can be used to reset and
> initialize HW controlled by the firmware.
>
> Signed-off-by: Nicolas Saenz Julienne
> Reviewed-by: Florian Fainelli
>
> ---
> Changes since v2:
> - Add include file for reset IDs
>
> Changes since v1:
> - Correct cells binding as per Florian's comment
> - Change compatible string to be more generic
>
> .../arm/bcm/raspberrypi,bcm2835-firmware.yaml | 21 +++
> .../reset/raspberrypi,firmware-reset.h| 13
> 2 files changed, 34 insertions(+)
> create mode 100644 include/dt-bindings/reset/raspberrypi,firmware-reset.h
>
> diff --git
> a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml
> b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml
> index b48ed875eb8e..23a885af3a28 100644
> ---
> a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml
> +++
> b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml
> @@ -39,6 +39,22 @@ properties:
>- compatible
>- "#clock-cells"
>
> + reset:
> +type: object
> +
> +properties:
> + compatible:
> +const: raspberrypi,firmware-reset
> +
> + "#reset-cells":
> +const: 1
> +description: >
> + The argument is the ID of the firmware reset line to affect.
> +
> +required:
> + - compatible
> + - "#reset-cells"
> +
> additionalProperties: false
>
> required:
> @@ -55,5 +71,10 @@ examples:
> compatible = "raspberrypi,firmware-clocks";
> #clock-cells = <1>;
> };
> +
> +reset: reset {
> +compatible = "raspberrypi,firmware-reset";
> +#reset-cells = <1>;
> +};
> };
> ...
> diff --git a/include/dt-bindings/reset/raspberrypi,firmware-reset.h
> b/include/dt-bindings/reset/raspberrypi,firmware-reset.h
> new file mode 100644
> index ..1a4f4c792723
> --- /dev/null
> +++ b/include/dt-bindings/reset/raspberrypi,firmware-reset.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2020 Nicolas Saenz Julienne
> + * Author: Nicolas Saenz Julienne
> + */
> +
> +#ifndef _DT_BINDINGS_RASPBERRYPI_FIRMWARE_RESET_H
> +#define _DT_BINDINGS_RASPBERRYPI_FIRMWARE_RESET_H
> +
> +#define RASPBERRYPI_FIRMWARE_RESET_ID_USB0
> +#define RASPBERRYPI_FIRMWARE_RESET_NUM_IDS 1
> +
> +#endif
Are there going to be any more firmware controlled resets in the future?
regards
Philipp
Re: [PATCH v6 3/9] reset: add BCM6345 reset controller driver
Hi Álvaro,
thank you for the patch. A few comments below:
On Wed, 2020-06-17 at 10:32 +0200, Álvaro Fernández Rojas wrote:
> Add support for resetting blocks through the Linux reset controller
> subsystem for BCM63xx SoCs.
>
> Signed-off-by: Álvaro Fernández Rojas
> Reviewed-by: Florian Fainelli
> ---
> v6: driver improvements:
> - use devm_platform_ioremap_resource.
> - simplify bcm6345_reset_probe return.
> - introduce and use to_bcm6345_reset function.
> v5: fix kbuild robot error (drop __init).
> v4: no changes.
> v3: using reset-simple isn't possible since sleeping after performing the
> reset is also needed.
> v2: add compatibility to reset-simple instead of adding a new driver.
>
> drivers/reset/Kconfig | 7 ++
> drivers/reset/Makefile| 1 +
> drivers/reset/reset-bcm6345.c | 132 ++
> 3 files changed, 140 insertions(+)
> create mode 100644 drivers/reset/reset-bcm6345.c
>
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index d9efbfd29646..9f1da978cef6 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -41,6 +41,13 @@ config RESET_BERLIN
> help
> This enables the reset controller driver for Marvell Berlin SoCs.
>
> +config RESET_BCM6345
> + bool "BCM6345 Reset Controller"
> + depends on BMIPS_GENERIC || COMPILE_TEST
> + default BMIPS_GENERIC
> + help
> + This enables the reset controller driver for BCM6345 SoCs.
> +
Please sort these alphabetically.
> config RESET_BRCMSTB
> tristate "Broadcom STB reset controller"
> depends on ARCH_BRCMSTB || COMPILE_TEST
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 249ed357c997..e642aae42f0f 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_ARCH_TEGRA) += tegra/
> obj-$(CONFIG_RESET_A10SR) += reset-a10sr.o
> obj-$(CONFIG_RESET_ATH79) += reset-ath79.o
> obj-$(CONFIG_RESET_AXS10X) += reset-axs10x.o
> +obj-$(CONFIG_RESET_BCM6345) += reset-bcm6345.o
This is the right place.
> obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
> obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
> obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
> diff --git a/drivers/reset/reset-bcm6345.c b/drivers/reset/reset-bcm6345.c
> new file mode 100644
> index ..3eedea226028
> --- /dev/null
> +++ b/drivers/reset/reset-bcm6345.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * BCM6345 Reset Controller Driver
> + *
> + * Copyright (C) 2020 Álvaro Fernández Rojas
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define BCM6345_RESET_NUM32
> +#define BCM6345_RESET_SLEEP_MIN_US 1
> +#define BCM6345_RESET_SLEEP_MAX_US 2
> +
> +struct bcm6345_reset {
> + struct reset_controller_dev rcdev;
> + void __iomem *base;
> + spinlock_t lock;
> +};
> +
> +static inline struct bcm6345_reset *
> +to_bcm6345_reset(struct reset_controller_dev *rcdev)
> +{
> + return container_of(rcdev, struct bcm6345_reset, rcdev);
> +}
> +
> +static void bcm6345_reset_update(struct reset_controller_dev *rcdev,
> + unsigned long id, bool assert)
> +{
> + struct bcm6345_reset *bcm6345_reset = to_bcm6345_reset(rcdev);
> + unsigned long flags;
> + uint32_t val;
> +
> + spin_lock_irqsave(_reset->lock, flags);
> + val = __raw_readl(bcm6345_reset->base);
> + if (assert)
> + val &= ~BIT(id);
> + else
> + val |= BIT(id);
> + __raw_writel(val, bcm6345_reset->base);
> + spin_unlock_irqrestore(_reset->lock, flags);
> +}
> +
> +static int bcm6345_reset_assert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + bcm6345_reset_update(rcdev, id, true);
> +
> + return 0;
> +}
> +
> +static int bcm6345_reset_deassert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + bcm6345_reset_update(rcdev, id, false);
> +
> + return 0;
These two could be shortened if you let bcm6345_reset_update() return 0:
return bcm6345_reset_update(rcdev, id, false);
> +}
> +
> +static int bcm6345_reset_reset(struct reset_controller_dev *rcdev,
> +unsigned long id)
> +{
> + bcm6345_reset_update(rcdev, id, true);
> + usleep_range(BCM6345_RESET_SLEEP_MIN_US,
> + BCM6345_RESET_SLEEP_MAX_US);
> +
> + bcm6345_reset_update(rcdev, id, false);
This second sleep is unusual:
> + usleep_range(BCM6345_RESET_SLEEP_MIN_US,
> + BCM6345_RESET_SLEEP_MAX_US);
Could you add a comment describing why it is needed?
Otherwise,
Reviewed-by: Philipp Zabel
regards
Philipp
Re: [PATCH v3 2/2] phy: bcm63xx-usbh: Add BCM63xx USBH driver
Hi Álvaro,
On Tue, 2020-06-16 at 20:45 +0200, Álvaro Fernández Rojas wrote:
> Add BCM63xx USBH PHY driver for BMIPS.
>
> Signed-off-by: Álvaro Fernández Rojas
> ---
> v3: introduce changes suggested by Florian:
> - Add support for device mode.
> v2: introduce changes suggested by Florian:
> - Drop OF dependency (use device_get_match_data).
> - Drop __initconst from variant tables.
> - Use devm_clk_get_optional.
>
> drivers/phy/broadcom/Kconfig| 9 +
> drivers/phy/broadcom/Makefile | 1 +
> drivers/phy/broadcom/phy-bcm63xx-usbh.c | 456
> 3 files changed, 466 insertions(+)
> create mode 100644 drivers/phy/broadcom/phy-bcm63xx-usbh.c
>
[...]
> diff --git a/drivers/phy/broadcom/phy-bcm63xx-usbh.c
> b/drivers/phy/broadcom/phy-bcm63xx-usbh.c
> new file mode 100644
> index ..584807205166
> --- /dev/null
> +++ b/drivers/phy/broadcom/phy-bcm63xx-usbh.c
> @@ -0,0 +1,456 @@
[...]
> + usbh->reset = devm_reset_control_get(dev, NULL);
Please use devm_reset_control_get_exclusive() instead when requesting
exclusive reset controls.
> + if (IS_ERR(usbh->reset)) {
> + if (PTR_ERR(usbh->reset) != -EPROBE_DEFER)
> + dev_err(dev, "failed to get reset\n");
> + return PTR_ERR(usbh->reset);
> + }
regards
Philipp
Re: [PATCH v4 2/2] usb: phy: Add USB3 PHY support for Intel LGM SoC
Hi Vadivel,
On Wed, 2020-06-17 at 11:58 +0800, Ramuthevar,Vadivel MuruganX wrote:
> From: Ramuthevar Vadivel Murugan
>
> Add support for USB PHY on Intel LGM SoC.
>
> Signed-off-by: Ramuthevar Vadivel Murugan
>
> ---
> drivers/usb/phy/Kconfig | 11 ++
> drivers/usb/phy/Makefile | 1 +
> drivers/usb/phy/phy-lgm-usb.c | 275
> ++
> 3 files changed, 287 insertions(+)
> create mode 100644 drivers/usb/phy/phy-lgm-usb.c
>
[...]
> diff --git a/drivers/usb/phy/phy-lgm-usb.c b/drivers/usb/phy/phy-lgm-usb.c
> new file mode 100644
> index ..3da772dfd736
> --- /dev/null
> +++ b/drivers/usb/phy/phy-lgm-usb.c
> @@ -0,0 +1,275 @@
[...]
> + for (i = 0; i < ARRAY_SIZE(CTL_RESETS); i++) {
> + resets[i] = devm_reset_control_get_exclusive(dev,
> CTL_RESETS[i]);
> + if (IS_ERR(resets[i])) {
> + dev_err(dev, "%s reset not found\n", CTL_RESETS[i]);
> + return PTR_ERR(resets[i]);
> + }
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(PHY_RESETS); i++) {
> + ta->resets[i] = devm_reset_control_get_exclusive(dev,
> PHY_RESETS[i]);
> + if (IS_ERR(ta->resets[i])) {
> + dev_err(dev, "%s reset not found\n", PHY_RESETS[i]);
> + return PTR_ERR(ta->resets[i]);
> + }
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(CTL_RESETS); i++)
> + reset_control_assert(resets[i]);
> +
> + for (i = 0; i < ARRAY_SIZE(PHY_RESETS); i++)
> + reset_control_assert(ta->resets[i]);
> + /*
> + * Out-of-band reset of the controller after PHY reset will cause
> + * controller malfunctioning, so we should use in-band controller
> + * reset only and leave the controller de-asserted here.
> + */
> + for (i = 0; i < ARRAY_SIZE(CTL_RESETS); i++)
> + reset_control_deassert(resets[i]);
This driver could probably benefit from a reset_bulk API similar to the
clk_bulk and regulator_bulk APIs, but that doesn't exist yet.
For the reset handling in this driver,
Reviewed-by: Philipp Zabel
regards
Philipp
Re: [PATCH v4 3/3] ARM: dts: bcm2711: Add HDMI DVP
Hi Maxime,
On Tue, 2020-06-16 at 14:16 +0200, Maxime Ripard wrote:
> Hi Nicolas,
>
> On Mon, Jun 15, 2020 at 06:26:19PM +0200, Nicolas Saenz Julienne wrote:
> > On Thu, 2020-06-11 at 11:23 +0200, Maxime Ripard wrote:
> > > Now that we have a driver for the DVP, let's add its DT node.
> > >
> > > Signed-off-by: Maxime Ripard
> > > ---
> >
> > I can take this patch, but I guess the rest should go trough the clock tree.
> > Is it OK with you?
>
> We have a build dependency on the reset framework for that driver, so it
> should rather go through the reset tree (or Philipp should make an
> immutable branch that the clk maintainers can merge).
I've prepared an immutable branch that these patches could be based on
and that could be merged into the clk tree:
The following changes since commit b3a9e3b9622ae10064826dccb4f7a52bd88c7407:
Linux 5.8-rc1 (2020-06-14 12:45:04 -0700)
are available in the Git repository at:
git://git.pengutronix.de/git/pza/linux reset/simple
for you to fetch changes up to a9701376ed0fb61a5be4bb438daf26bd9cfa24b5:
reset: simple: Add reset callback (2020-06-16 14:19:57 +0200)
Maxime Ripard (2):
reset: Move reset-simple header out of drivers/reset
reset: simple: Add reset callback
drivers/reset/reset-simple.c| 23 +--
drivers/reset/reset-socfpga.c | 3 +--
drivers/reset/reset-sunxi.c | 3 +--
drivers/reset/reset-uniphier-glue.c | 3 +--
{drivers => include/linux}/reset/reset-simple.h | 7 +++
5 files changed, 31 insertions(+), 8 deletions(-)
rename {drivers => include/linux}/reset/reset-simple.h (74%)
regards
Philipp
Re: [PATCH 1/4] spi: bcm63xx-spi: add reset support
Hi Álvaro,
On Mon, 2020-06-15 at 10:03 +0200, Álvaro Fernández Rojas wrote:
> bcm63xx arch resets the SPI controller at early boot. However, bmips arch
> needs to perform a reset when probing the driver.
>
> Signed-off-by: Álvaro Fernández Rojas
> ---
> drivers/spi/spi-bcm63xx.c | 17 +
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/spi/spi-bcm63xx.c b/drivers/spi/spi-bcm63xx.c
> index 0f1b10a4ef0c..8ab04affaf7b 100644
> --- a/drivers/spi/spi-bcm63xx.c
> +++ b/drivers/spi/spi-bcm63xx.c
> @@ -18,6 +18,7 @@
> #include
> #include
> #include
> +#include
>
> /* BCM 6338/6348 SPI core */
> #define SPI_6348_RSET_SIZE 64
> @@ -493,6 +494,7 @@ static int bcm63xx_spi_probe(struct platform_device *pdev)
> struct bcm63xx_spi *bs;
> int ret;
> u32 num_cs = BCM63XX_SPI_MAX_CS;
> + struct reset_control *reset;
>
> if (dev->of_node) {
> const struct of_device_id *match;
> @@ -529,6 +531,15 @@ static int bcm63xx_spi_probe(struct platform_device
> *pdev)
> return PTR_ERR(clk);
> }
>
> + reset = devm_reset_control_get(dev, NULL);
Please use devm_reset_control_get_exclusive(), same for patch 3.
With that changed,
Reviewed-by: Philipp Zabel
regards
Philipp
Re: [PATCH 14/29] dt: Fix broken references to renamed docs
On Mon, 2020-06-15 at 08:46 +0200, Mauro Carvalho Chehab wrote: > Some files got renamed. Those were all fixed automatically by > > ./scripts/documentation-file-ref-check --fix > > Signed-off-by: Mauro Carvalho Chehab > --- > Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt | 2 +- > Documentation/devicetree/bindings/display/imx/fsl-imx-drm.txt | 4 ++-- > Documentation/devicetree/bindings/display/imx/ldb.txt | 4 ++-- > Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.txt | 2 +- > MAINTAINERS | 4 ++-- > 5 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt > index 715047444391..10b8459e49f8 100644 > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt > @@ -47,7 +47,7 @@ Required properties: > _mu1 1 2 > _mu1 1 3 > _mu1 3 3>; > - See Documentation/devicetree/bindings/mailbox/fsl,mu.txt > + See Documentation/devicetree/bindings/mailbox/fsl,mu.yaml > for detailed mailbox binding. > > Note: Each mu which supports general interrupt should have an alias correctly > diff --git a/Documentation/devicetree/bindings/display/imx/fsl-imx-drm.txt > b/Documentation/devicetree/bindings/display/imx/fsl-imx-drm.txt > index 5bf77f6dd19d..5a99490c17b9 100644 > --- a/Documentation/devicetree/bindings/display/imx/fsl-imx-drm.txt > +++ b/Documentation/devicetree/bindings/display/imx/fsl-imx-drm.txt > @@ -68,7 +68,7 @@ Required properties: >datasheet > - clocks : phandle to the PRE axi clock input, as described >in Documentation/devicetree/bindings/clock/clock-bindings.txt and > - Documentation/devicetree/bindings/clock/imx6q-clock.txt. > + Documentation/devicetree/bindings/clock/imx6q-clock.yaml. > - clock-names: should be "axi" > - interrupts: should contain the PRE interrupt > - fsl,iram: phandle pointing to the mmio-sram device node, that should be > @@ -94,7 +94,7 @@ Required properties: >datasheet > - clocks : phandles to the PRG ipg and axi clock inputs, as described >in Documentation/devicetree/bindings/clock/clock-bindings.txt and > - Documentation/devicetree/bindings/clock/imx6q-clock.txt. > + Documentation/devicetree/bindings/clock/imx6q-clock.yaml. > - clock-names: should be "ipg" and "axi" > - fsl,pres: phandles to the PRE units attached to this PRG, with the fixed >PRE as the first entry and the muxable PREs following. > diff --git a/Documentation/devicetree/bindings/display/imx/ldb.txt > b/Documentation/devicetree/bindings/display/imx/ldb.txt > index 38c637fa39dd..8e6e7d797943 100644 > --- a/Documentation/devicetree/bindings/display/imx/ldb.txt > +++ b/Documentation/devicetree/bindings/display/imx/ldb.txt > @@ -30,8 +30,8 @@ Required properties: > "di2_sel" - IPU2 DI0 mux > "di3_sel" - IPU2 DI1 mux > The needed clock numbers for each are documented in > -Documentation/devicetree/bindings/clock/imx5-clock.txt, and in > -Documentation/devicetree/bindings/clock/imx6q-clock.txt. > +Documentation/devicetree/bindings/clock/imx5-clock.yaml, and in > +Documentation/devicetree/bindings/clock/imx6q-clock.yaml. > > Optional properties: > - pinctrl-names : should be "default" on i.MX53, not used on i.MX6q Reviewed-by: Philipp Zabel regards Philipp
Re: [PATCH 3/7] reset: add BCM6345 reset controller driver
Hi Álvaro,
On Wed, 2020-06-10 at 08:08 +0200, Álvaro Fernández Rojas wrote:
> Hi Florian,
>
> > El 9 jun 2020, a las 22:17, Florian Fainelli
> > escribió:
> >
> >
> >
> > On 6/9/2020 9:41 AM, Álvaro Fernández Rojas wrote:
> > > > > > If you can do without this, with I think this driver could be made
> > > > > > to
> > > > > > use reset-simple.
> > > > >
> > > > > Yes, but only if I can add reset support with a configurable sleep
> > > > > range to reset-simple. Is this possible?
> > > >
> > > > I should have mentioned, support for this is on the reset/next branch:
> > > >
> > > > git://git.pengutronix.de/pza/linux.git reset/next
> > >
> > > Yes, but reset_us was only added to reset_simple_data, so there’s no way
> > > to fill that value from reset_simple_devdata or device tree, right?
> >
> > Not that I can see, but you could certainly extend it here:
> >
> > if (devdata) {
> > reg_offset = devdata->reg_offset;
> > if (devdata->nr_resets)
> > data->rcdev.nr_resets = devdata->nr_resets;
> > data->active_low = devdata->active_low;
> > data->status_active_low = devdata->status_active_low;
> > }
>
> Yes, I would extend it there too,
You are right, reset_us is missing from reset_simple_devdata.
> but I was just saying that it’s a bit strange that it was only added
> to reset_simple_data without any way to fill the value.
The patch was added for the benefit of drivers that register their own
reset controller using reset_simple_data/ops, like sunxi or socfpga.
This might be considered an oversight, but until now there was no user
inside reset-simple.c.
regards
Philipp
Re: [PATCH 3/7] reset: add BCM6345 reset controller driver
Hi Álvaro,
On Tue, 2020-06-09 at 17:14 +0200, Álvaro Fernández Rojas wrote:
> Hi Philipp,
>
> > El 9 jun 2020, a las 17:06, Philipp Zabel escribió:
> >
> > Hi Álvaro,
> >
> > On Tue, 2020-06-09 at 15:42 +0200, Álvaro Fernández Rojas wrote:
> > > Add support for resetting blocks through the Linux reset controller
> > > subsystem for BCM63xx SoCs.
> > >
> > > Signed-off-by: Álvaro Fernández Rojas
> > > ---
> > > drivers/reset/Kconfig | 7 ++
> > > drivers/reset/Makefile| 1 +
> > > drivers/reset/reset-bcm6345.c | 149 ++
> > > 3 files changed, 157 insertions(+)
> > > create mode 100644 drivers/reset/reset-bcm6345.c
> > >
> > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > > index d9efbfd29646..9f1da978cef6 100644
> > > --- a/drivers/reset/Kconfig
> > > +++ b/drivers/reset/Kconfig
> > > @@ -41,6 +41,13 @@ config RESET_BERLIN
> > > help
> > > This enables the reset controller driver for Marvell Berlin SoCs.
> > >
> > > +config RESET_BCM6345
> > > + bool "BCM6345 Reset Controller"
> > > + depends on BMIPS_GENERIC || COMPILE_TEST
> > > + default BMIPS_GENERIC
> > > + help
> > > + This enables the reset controller driver for BCM6345 SoCs.
> > > +
> > > config RESET_BRCMSTB
> > > tristate "Broadcom STB reset controller"
> > > depends on ARCH_BRCMSTB || COMPILE_TEST
> > > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> > > index 249ed357c997..e642aae42f0f 100644
> > > --- a/drivers/reset/Makefile
> > > +++ b/drivers/reset/Makefile
> > > @@ -6,6 +6,7 @@ obj-$(CONFIG_ARCH_TEGRA) += tegra/
> > > obj-$(CONFIG_RESET_A10SR) += reset-a10sr.o
> > > obj-$(CONFIG_RESET_ATH79) += reset-ath79.o
> > > obj-$(CONFIG_RESET_AXS10X) += reset-axs10x.o
> > > +obj-$(CONFIG_RESET_BCM6345) += reset-bcm6345.o
> > > obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
> > > obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
> > > obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
> > > diff --git a/drivers/reset/reset-bcm6345.c b/drivers/reset/reset-bcm6345.c
> > > new file mode 100644
> > > index ..088b7fdb896b
> > > --- /dev/null
> > > +++ b/drivers/reset/reset-bcm6345.c
> > > @@ -0,0 +1,149 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * BCM6345 Reset Controller Driver
> > > + *
> > > + * Copyright (C) 2020 Álvaro Fernández Rojas
> > > + */
> > > +
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +#define BCM6345_RESET_NUM32
> > > +#define BCM6345_RESET_SLEEP_MIN_US 1
> > > +#define BCM6345_RESET_SLEEP_MAX_US 2
> > > +
> > > +struct bcm6345_reset {
> > > + struct reset_controller_dev rcdev;
> > > + void __iomem *base;
> > > + spinlock_t lock;
> > > +};
> > > +
> > > +static int bcm6345_reset_update(struct bcm6345_reset *bcm6345_reset,
> > > + unsigned long id, bool assert)
> > > +{
> > > + uint32_t val;
> > > +
> > > + val = __raw_readl(bcm6345_reset->base);
> > > + if (assert)
> > > + val &= ~BIT(id);
> > > + else
> > > + val |= BIT(id);
> > > + __raw_writel(val, bcm6345_reset->base);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int bcm6345_reset_assert(struct reset_controller_dev *rcdev,
> > > + unsigned long id)
> > > +{
> > > + struct bcm6345_reset *bcm6345_reset =
> > > + container_of(rcdev, struct bcm6345_reset, rcdev);
> > > + unsigned long flags;
> > > +
> > > + spin_lock_irqsave(_reset->lock, flags);
> > > + bcm6345_reset_update(bcm6345_reset, id, true);
> > > + spin_unlock_irqrestore(_reset->lock, flags);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int bcm6345_reset_deassert(struct reset_controller_dev *rcdev,
> > > + unsigned long id)
> > > +{
> > > + struct bcm6345_reset *bcm6345_reset =
> > > + container_of(rcdev, struct bcm6345_reset, rcdev);
> > >
Re: [PATCH 3/7] reset: add BCM6345 reset controller driver
Hi Álvaro,
On Tue, 2020-06-09 at 15:42 +0200, Álvaro Fernández Rojas wrote:
> Add support for resetting blocks through the Linux reset controller
> subsystem for BCM63xx SoCs.
>
> Signed-off-by: Álvaro Fernández Rojas
> ---
> drivers/reset/Kconfig | 7 ++
> drivers/reset/Makefile| 1 +
> drivers/reset/reset-bcm6345.c | 149 ++
> 3 files changed, 157 insertions(+)
> create mode 100644 drivers/reset/reset-bcm6345.c
>
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index d9efbfd29646..9f1da978cef6 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -41,6 +41,13 @@ config RESET_BERLIN
> help
> This enables the reset controller driver for Marvell Berlin SoCs.
>
> +config RESET_BCM6345
> + bool "BCM6345 Reset Controller"
> + depends on BMIPS_GENERIC || COMPILE_TEST
> + default BMIPS_GENERIC
> + help
> + This enables the reset controller driver for BCM6345 SoCs.
> +
> config RESET_BRCMSTB
> tristate "Broadcom STB reset controller"
> depends on ARCH_BRCMSTB || COMPILE_TEST
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 249ed357c997..e642aae42f0f 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_ARCH_TEGRA) += tegra/
> obj-$(CONFIG_RESET_A10SR) += reset-a10sr.o
> obj-$(CONFIG_RESET_ATH79) += reset-ath79.o
> obj-$(CONFIG_RESET_AXS10X) += reset-axs10x.o
> +obj-$(CONFIG_RESET_BCM6345) += reset-bcm6345.o
> obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
> obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
> obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
> diff --git a/drivers/reset/reset-bcm6345.c b/drivers/reset/reset-bcm6345.c
> new file mode 100644
> index ..088b7fdb896b
> --- /dev/null
> +++ b/drivers/reset/reset-bcm6345.c
> @@ -0,0 +1,149 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * BCM6345 Reset Controller Driver
> + *
> + * Copyright (C) 2020 Álvaro Fernández Rojas
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define BCM6345_RESET_NUM32
> +#define BCM6345_RESET_SLEEP_MIN_US 1
> +#define BCM6345_RESET_SLEEP_MAX_US 2
> +
> +struct bcm6345_reset {
> + struct reset_controller_dev rcdev;
> + void __iomem *base;
> + spinlock_t lock;
> +};
> +
> +static int bcm6345_reset_update(struct bcm6345_reset *bcm6345_reset,
> + unsigned long id, bool assert)
> +{
> + uint32_t val;
> +
> + val = __raw_readl(bcm6345_reset->base);
> + if (assert)
> + val &= ~BIT(id);
> + else
> + val |= BIT(id);
> + __raw_writel(val, bcm6345_reset->base);
> +
> + return 0;
> +}
> +
> +static int bcm6345_reset_assert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct bcm6345_reset *bcm6345_reset =
> + container_of(rcdev, struct bcm6345_reset, rcdev);
> + unsigned long flags;
> +
> + spin_lock_irqsave(_reset->lock, flags);
> + bcm6345_reset_update(bcm6345_reset, id, true);
> + spin_unlock_irqrestore(_reset->lock, flags);
> +
> + return 0;
> +}
> +
> +static int bcm6345_reset_deassert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct bcm6345_reset *bcm6345_reset =
> + container_of(rcdev, struct bcm6345_reset, rcdev);
> + unsigned long flags;
> +
> + spin_lock_irqsave(_reset->lock, flags);
> + bcm6345_reset_update(bcm6345_reset, id, false);
> + spin_unlock_irqrestore(_reset->lock, flags);
> +
> + return 0;
> +}
> +
> +static int bcm6345_reset_reset(struct reset_controller_dev *rcdev,
> +unsigned long id)
> +{
> + struct bcm6345_reset *bcm6345_reset =
> + container_of(rcdev, struct bcm6345_reset, rcdev);
> + unsigned long flags;
> +
> + spin_lock_irqsave(_reset->lock, flags);
> + usleep_range(BCM6345_RESET_SLEEP_MIN_US,
> + BCM6345_RESET_SLEEP_MAX_US);
What is the purpose of sleeping before reset assertion?
If you can do without this, with I think this driver could be made to
use reset-simple.
regards
Philipp
Re: [PATCH v1 2/2] usb: phy: Add USB3 PHY support for Intel LGM SoC
Hi Ramuthevar,
On Tue, 2020-06-09 at 19:08 +0800, Ramuthevar,Vadivel MuruganX wrote:
> From: Ramuthevar Vadivel Murugan
>
> Add support for USB PHY on Intel LGM SoC.
>
> Signed-off-by: Ramuthevar Vadivel Murugan
>
> ---
> drivers/usb/phy/Kconfig | 11 ++
> drivers/usb/phy/Makefile | 1 +
> drivers/usb/phy/phy-lgm-usb.c | 269
> ++
> 3 files changed, 281 insertions(+)
> create mode 100644 drivers/usb/phy/phy-lgm-usb.c
>
> diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
> index 4b3fa78995cf..95f2e737d663 100644
> --- a/drivers/usb/phy/Kconfig
> +++ b/drivers/usb/phy/Kconfig
> @@ -192,4 +192,15 @@ config JZ4770_PHY
> This driver provides PHY support for the USB controller found
> on the JZ4770 SoC from Ingenic.
>
> +config USB_LGM_PHY
> + tristate "INTEL Lightning Mountain USB PHY Driver"
> + depends on USB_SUPPORT
> + select USB_PHY
> + select REGULATOR
> + select REGULATOR_FIXED_VOLTAGE
> + help
> + Enable this to support Intel DWC3 PHY USB phy. This driver provides
> + interface to interact with USB GEN-II and USB 3.x PHY that is part
> + of the Intel network SOC.
> +
> endmenu
> diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
> index b352bdbe8712..ef5345164e10 100644
> --- a/drivers/usb/phy/Makefile
> +++ b/drivers/usb/phy/Makefile
> @@ -25,3 +25,4 @@ obj-$(CONFIG_USB_ULPI) += phy-ulpi.o
> obj-$(CONFIG_USB_ULPI_VIEWPORT) += phy-ulpi-viewport.o
> obj-$(CONFIG_KEYSTONE_USB_PHY) += phy-keystone.o
> obj-$(CONFIG_JZ4770_PHY) += phy-jz4770.o
> +obj-$(CONFIG_USB_LGM_PHY)+= phy-lgm-usb.o
> diff --git a/drivers/usb/phy/phy-lgm-usb.c b/drivers/usb/phy/phy-lgm-usb.c
> new file mode 100644
> index ..66cb327b7b71
> --- /dev/null
> +++ b/drivers/usb/phy/phy-lgm-usb.c
> @@ -0,0 +1,269 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Intel LGM USB PHY driver
> + *
> + * Copyright (C) 2020 Intel Corporation.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define CTRL1_OFFSET 0x14
> +#define SRAM_EXT_LD_DONE BIT(25)
> +#define SRAM_INIT_DONE BIT(26)
> +
> +#define TCPC_OFFSET 0x1014
> +#define TCPC_MUX_CTL GENMASK(1, 0)
> +#define MUX_NC 0
> +#define MUX_USB 1
> +#define MUX_DP 2
> +#define MUX_USBDP3
> +#define TCPC_FLIPPED BIT(2)
> +#define TCPC_LOW_POWER_ENBIT(3)
> +#define TCPC_VALID BIT(4)
> +#define TCPC_DISCONN \
> + (TCPC_VALID | FIELD_PREP(TCPC_MUX_CTL, MUX_NC) | TCPC_LOW_POWER_EN)
> +
> +static const char *const PHY_RESETS[] = { "phy31", "phy", };
> +static const char *const CTL_RESETS[] = { "apb", "ctrl", };
> +
> +struct tca_apb {
> + struct reset_control *resets[ARRAY_SIZE(PHY_RESETS)];
> + struct regulator *vbus;
> + struct work_struct wk;
> + struct usb_phy phy;
> +
> + bool phy_initialized;
> + bool connected;
> +};
> +
> +static int get_flipped(struct tca_apb *ta, bool *flipped)
> +{
> + union extcon_property_value property;
> + int ret;
> +
> + ret = extcon_get_property(ta->phy.edev, EXTCON_USB_HOST,
> + EXTCON_PROP_USB_TYPEC_POLARITY, );
> + if (ret) {
> + dev_err(ta->phy.dev, "no polarity property from extcon\n");
> + return false;
> + }
> +
> + *flipped = property.intval;
> +
> + return *flipped;
> +}
> +
> +static int phy_init(struct usb_phy *phy)
> +{
> + struct tca_apb *ta = container_of(phy, struct tca_apb, phy);
> + void __iomem *ctrl1 = phy->io_priv + CTRL1_OFFSET;
> + int val, ret, i;
> +
> + if (ta->phy_initialized)
> + return 0;
> +
> + for (i = 0; i < ARRAY_SIZE(PHY_RESETS); i++)
> + reset_control_deassert(ta->resets[i]);
> +
> + ret = readl_poll_timeout(ctrl1, val, val & SRAM_INIT_DONE,
> + 10, 10 * 1000);
> + if (IS_ERR(ret)) {
> + dev_err(ta->phy.dev, "SRAM init failed, 0x%x\n", val);
> + return PTR_ERR(ret);
> + }
> +
> + writel(readl(ctrl1) | SRAM_EXT_LD_DONE, ctrl1);
> +
> + ta->phy_initialized = true;
> + if (!ta->phy.edev)
> + return phy->set_vbus(phy, true);
> +
> + schedule_work(>wk);
> +
> + return 0;
> +}
> +
> +static void phy_shutdown(struct usb_phy *phy)
> +{
> + struct tca_apb *ta = container_of(phy, struct tca_apb, phy);
> + int i;
> +
> + if (!ta->phy_initialized)
> + return;
> +
> + ta->phy_initialized = false;
> + flush_work(>wk);
> + ta->phy.set_vbus(>phy, false);
> + if (ta->connected) {
> + ta->connected = false;
> + writel(TCPC_DISCONN, ta->phy.io_priv + TCPC_OFFSET);
Re: [PATCH 5/9] usb: xhci-pci: Add support for reset controllers
Hi Nicolas, On Tue, 2020-06-09 at 13:18 +0200, Nicolas Saenz Julienne wrote: > Hi Florian, thanks for the reviews! > > On Mon, 2020-06-08 at 12:43 -0700, Florian Fainelli wrote: > > On 6/8/2020 12:26 PM, Nicolas Saenz Julienne wrote: > > > Some atypical users of xhci-pci might need to manually reset their xHCI > > > controller before starting the HCD setup. Check if a reset controller > > > device is available to the PCI bus and trigger a reset. > > > > > > Signed-off-by: Nicolas Saenz Julienne > > > --- > > > drivers/usb/host/xhci-pci.c | 9 + > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c > > > index ef513c2fb843..45f70facdfcd 100644 > > > --- a/drivers/usb/host/xhci-pci.c > > > +++ b/drivers/usb/host/xhci-pci.c [...] > > > @@ -347,6 +349,13 @@ static int xhci_pci_probe(struct pci_dev *dev, const > > > struct pci_device_id *id) > > > return retval; > > > } > > > > > > + reset = devm_reset_control_get(>bus->dev, NULL); > > > > Should not this be devm_reset_control_get_optional()? > > Yes, you're right. Please use devm_reset_control_get_optional_exclusive() while you're at it. regards Philipp
Re: [PATCH V3] dt-bindings: reset: Convert i.MX reset to json-schema
Hi Anson, On Tue, 2020-05-19 at 11:42 +0800, Anson Huang wrote: > Convert the i.MX reset binding to DT schema format using json-schema. > > Signed-off-by: Anson Huang > Reviewed-by: Dong Aisheng Thank you, applied to reset/next. regards Philipp
Re: [PATCH V2] dt-bindings: reset: Convert i.MX7 reset to json-schema
Hi Anson, On Mon, 2020-05-11 at 19:57 +0800, Anson Huang wrote: > Convert the i.MX7 reset binding to DT schema format using json-schema. > > Signed-off-by: Anson Huang Thank you, applied to reset/next with Rob's and Dong's R-b. regards Philipp
Re: [PATCH v3 002/105] reset: simple: Add reset callback
Hi Maxime, On Wed, 2020-05-27 at 17:47 +0200, Maxime Ripard wrote: > The reset-simple code lacks a reset callback that is still pretty easy to > implement. The only real thing to consider is the delay needed for a device > to be reset, so let's expose that as part of the reset-simple driver data. > > Cc: Philipp Zabel > Reviewed-by: Philipp Zabel > Signed-off-by: Maxime Ripard Thank you, I've applied patches 1 & 2 to the reset/next branch. regards Philipp
Re: [PATCH] media: coda: Fix runtime PM imbalance in coda_probe
Hi Dinghao, thank you for the patch! The first part is fine, but I think the second part is not necessary, see below: On Sat, May 23, 2020 at 06:03:32PM +0800, Dinghao Liu wrote: > When coda_firmware_request() returns an error code, > a pairing runtime PM usage counter decrement is needed > to keep the counter balanced. > > Also, the caller expects coda_probe() to increase PM > usage counter, there should be a refcount decrement > in coda_remove() to keep the counter balanced. coda_probe() increments the usage counter only until coda_fw_callback() decrements it again. Where is the imbalance? > Signed-off-by: Dinghao Liu > --- > drivers/media/platform/coda/coda-common.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/media/platform/coda/coda-common.c > b/drivers/media/platform/coda/coda-common.c > index d0d093dd8f7c..550e9a1266da 100644 > --- a/drivers/media/platform/coda/coda-common.c > +++ b/drivers/media/platform/coda/coda-common.c > @@ -3119,6 +3119,8 @@ static int coda_probe(struct platform_device *pdev) > return 0; > > err_alloc_workqueue: > + pm_runtime_disable(>dev); > + pm_runtime_put_noidle(>dev); These seem right, they balance out the pm_runtime_enable() and pm_runtime_get_noresume() right before the error. > destroy_workqueue(dev->workqueue); > err_v4l2_register: > v4l2_device_unregister(>v4l2_dev); > @@ -3136,6 +3138,7 @@ static int coda_remove(struct platform_device *pdev) > } > if (dev->m2m_dev) > v4l2_m2m_release(dev->m2m_dev); > + pm_runtime_put_noidle(>dev); I think this is incorrect. There is one pm_runtime_get_noresume() in coda_probe(), and one pm_runtime_put_sync() in coda_fw_callback(). By the time coda_remove() is called, balance is already restored. regards Philipp
Re: [PATCH 07/15] PCI: brcmstb: Add control of rescal reset
Hi Jim,
On Tue, May 19, 2020 at 04:34:05PM -0400, Jim Quinlan wrote:
> From: Jim Quinlan
>
> Some STB chips have a special purpose reset controller named
> RESCAL (reset calibration). This commit adds the control
> of RESCAL as well as the ability to start and stop its
> operation for PCIe HW.
>
> Signed-off-by: Jim Quinlan
> ---
> drivers/pci/controller/pcie-brcmstb.c | 81 ++-
> 1 file changed, 80 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c
> b/drivers/pci/controller/pcie-brcmstb.c
> index 2c470104ba38..0787e8f6f7e5 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
[...]
> @@ -1100,6 +1164,21 @@ static int brcm_pcie_probe(struct platform_device
> *pdev)
> dev_err(>dev, "could not enable clock\n");
> return ret;
> }
> + pcie->rescal = devm_reset_control_get_shared(>dev, "rescal");
> + if (IS_ERR(pcie->rescal)) {
> + if (PTR_ERR(pcie->rescal) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> + pcie->rescal = NULL;
This is effectively an optional reset control, so it is better to use:
↵
pcie->rescal = devm_reset_control_get_optional_shared(>dev,
"rescal");↵
if (IS_ERR(pcie->rescal))
return PTR_ERR(pcie->rescal);
> + } else {
> + ret = reset_control_deassert(pcie->rescal);
> + if (ret)
> + dev_err(>dev, "failed to deassert 'rescal'\n");
> + }
reset_control_* can handle rstc == NULL parameters for optional reset
controls, so this can be done unconditionally:
ret = reset_control_deassert(pcie->rescal);↵
if (ret)↵
dev_err(>dev, "failed to deassert 'rescal'\n");↵
Is rescal handled by the reset-brcmstb-rescal driver? Since that only
implements the .reset op, I would expect reset_control_reset() here.
Otherwise this looks like it'd be missing a reset_control_assert in
remove.
regards
Philipp
Re: [PATCH 02/15] ahci_brcm: fix use of BCM7216 reset controller
Hi Jim,
On Tue, May 19, 2020 at 04:34:00PM -0400, Jim Quinlan wrote:
> From: Jim Quinlan
>
> A reset controller "rescal" is shared between the AHCI driver
> and the PCIe driver for the BrcmSTB 7216 chip. The code is
> modified to allow this sharing and to deassert() properly.
>
> Signed-off-by: Jim Quinlan
> ---
> drivers/ata/ahci_brcm.c | 14 +-
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
> index 6853dbb4131d..a3c32fc29e9c 100644
> --- a/drivers/ata/ahci_brcm.c
> +++ b/drivers/ata/ahci_brcm.c
> @@ -428,7 +428,6 @@ static int brcm_ahci_probe(struct platform_device *pdev)
> {
> const struct of_device_id *of_id;
> struct device *dev = >dev;
> - const char *reset_name = NULL;
> struct brcm_ahci_priv *priv;
> struct ahci_host_priv *hpriv;
> struct resource *res;
> @@ -452,11 +451,11 @@ static int brcm_ahci_probe(struct platform_device *pdev)
>
> /* Reset is optional depending on platform and named differently */
> if (priv->version == BRCM_SATA_BCM7216)
> - reset_name = "rescal";
> + priv->rcdev = devm_reset_control_get_shared(>dev,
> + "rescal");
With this change the "rescal" reset control is not optional anymore.
Please use devm_reset_control_get_optional_shared() or change the
comment.
> else
> - reset_name = "ahci";
> -
> - priv->rcdev = devm_reset_control_get_optional(>dev, reset_name);
> + priv->rcdev = devm_reset_control_get_optional(>dev,
> + "ahci");
You can use devm_reset_control_get_optional_exclusive() here to make it
clear this requests an exclusive control.
regards
Philipp
Re: [PATCH] dt-bindings: reset: Convert i.MX reset to json-schema
Hi Anson,
On Tue, 2020-05-12 at 10:23 +0800, Anson Huang wrote:
> Convert the i.MX reset binding to DT schema format using json-schema.
>
> Signed-off-by: Anson Huang
thank you for the conversion.
> ---
> .../devicetree/bindings/reset/fsl,imx-src.txt | 49 --
> .../devicetree/bindings/reset/fsl,imx-src.yaml | 58
> ++
> 2 files changed, 58 insertions(+), 49 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/reset/fsl,imx-src.txt
> create mode 100644 Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
>
> diff --git a/Documentation/devicetree/bindings/reset/fsl,imx-src.txt
> b/Documentation/devicetree/bindings/reset/fsl,imx-src.txt
> deleted file mode 100644
> index 6ed79e6..000
> --- a/Documentation/devicetree/bindings/reset/fsl,imx-src.txt
> +++ /dev/null
> @@ -1,49 +0,0 @@
> -Freescale i.MX System Reset Controller
> -==
> -
> -Please also refer to reset.txt in this directory for common reset
> -controller binding usage.
> -
> -Required properties:
> -- compatible: Should be "fsl,-src"
> -- reg: should be register base and length as documented in the
> - datasheet
> -- interrupts: Should contain SRC interrupt and CPU WDOG interrupt,
> - in this order.
> -- #reset-cells: 1, see below
> -
> -example:
> -
> -src: src@20d8000 {
> -compatible = "fsl,imx6q-src";
> -reg = <0x020d8000 0x4000>;
> -interrupts = <0 91 0x04 0 96 0x04>;
> -#reset-cells = <1>;
> -};
> -
> -Specifying reset lines connected to IP modules
> -==
> -
> -The system reset controller can be used to reset the GPU, VPU,
> -IPU, and OpenVG IP modules on i.MX5 and i.MX6 ICs. Those device
> -nodes should specify the reset line on the SRC in their resets
> -property, containing a phandle to the SRC device node and a
> -RESET_INDEX specifying which module to reset, as described in
> -reset.txt
> -
> -example:
> -
> -ipu1: ipu@240 {
> -resets = < 2>;
> -};
> -ipu2: ipu@280 {
> -resets = < 4>;
> -};
> -
> -The following RESET_INDEX values are valid for i.MX5:
> -GPU_RESET 0
> -VPU_RESET 1
> -IPU1_RESET2
> -OPEN_VG_RESET 3
> -The following additional RESET_INDEX value is valid for i.MX6:
> -IPU2_RESET4
> diff --git a/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
> b/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
> new file mode 100644
> index 000..276a533
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/reset/fsl,imx-src.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Freescale i.MX System Reset Controller
> +
> +maintainers:
> + - Philipp Zabel
> +
> +description: |
> + The system reset controller can be used to reset the GPU, VPU,
> + IPU, and OpenVG IP modules on i.MX5 and i.MX6 ICs. Those device
> + nodes should specify the reset line on the SRC in their resets
> + property, containing a phandle to the SRC device node and a
> + RESET_INDEX specifying which module to reset, as described in
> + reset.txt
> +
> + The following RESET_INDEX values are valid for i.MX5:
> +GPU_RESET 0
> +VPU_RESET 1
> +IPU1_RESET2
> +OPEN_VG_RESET 3
> + The following additional RESET_INDEX value is valid for i.MX6:
> +IPU2_RESET4
> +
> +properties:
> + compatible:
> +items:
> + - const: "fsl,imx51-src"
"fsl,imx51-src" is the only compatible the driver matches on, but we
have these combinations on compatible SRCs in the device trees:
"fsl,imx50-src", "fsl,imx51-src"
"fsl,imx51-src"
"fsl,imx53-src", "fsl,imx51-src"
"fsl,imx6q-src", "fsl,imx51-src"
"fsl,imx6sl-src", "fsl,imx51-src"
"fsl,imx6sll-src", "fsl,imx51-src"
"fsl,imx6sx-src", "fsl,imx51-src"
"fsl,imx6ul-src", "fsl,imx51-src"
That could be described using oneOf and and an items list of const
values per SoC like in the qcom bindings.
regards
Philipp
Re: [PATCH v3 04/11] PCI: qcom: add missing reset for ipq806x
Hi Ansuel,
On Fri, May 01, 2020 at 12:06:11AM +0200, Ansuel Smith wrote:
> Add missing ext reset used by ipq8064 SoC in PCIe qcom driver.
>
> Fixes: 82a823833f4e PCI: qcom: Add Qualcomm PCIe controller driver
> Signed-off-by: Sham Muthayyan
> Signed-off-by: Ansuel Smith
> Cc: sta...@vger.kernel.org # v4.5+
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c
> b/drivers/pci/controller/dwc/pcie-qcom.c
> index 7a8901efc031..921030a64bab 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
[...]
> @@ -347,6 +353,12 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
> goto err_deassert_ahb;
> }
>
> + ret = reset_control_deassert(res->ext_reset);
> + if (ret) {
> + dev_err(dev, "cannot assert ext reset\n");
^
This probably should say "cannot deassert ext reset". Apart from this,
Reviewed-by: Philipp Zabel
regards
Philipp
Re: [PATCH] dt-bindings: reset: meson: add gxl internal dac reset
Hi Jerome, On Wed, 2020-05-06 at 15:50 +0200, Jerome Brunet wrote: > On Tue 14 Apr 2020 at 10:28, Jerome Brunet wrote: > > > On Thu 23 Jan 2020 at 11:13, Philipp Zabel wrote: > > > > > On Wed, 2020-01-22 at 10:25 +0100, Jerome Brunet wrote: > > > > Add the reset line of the internal DAC found on the amlogic gxl SoC > > > > family > > > > > > > > Signed-off-by: Jerome Brunet > > > > --- > > > > include/dt-bindings/reset/amlogic,meson-gxbb-reset.h | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/include/dt-bindings/reset/amlogic,meson-gxbb-reset.h > > > > b/include/dt-bindings/reset/amlogic,meson-gxbb-reset.h > > > > index ea5058618863..883bfd3bcbad 100644 > > > > --- a/include/dt-bindings/reset/amlogic,meson-gxbb-reset.h > > > > +++ b/include/dt-bindings/reset/amlogic,meson-gxbb-reset.h > > > > @@ -69,7 +69,7 @@ > > > > #define RESET_SYS_CPU_L2 58 > > > > #define RESET_SYS_CPU_P59 > > > > #define RESET_SYS_CPU_MBIST60 > > > > -/* 61 */ > > > > +#define RESET_ACODEC 61 > > > > /* 62 */ > > > > /* 63 */ > > > > /* RESET2 */ > > > > > > Thank you, applied to reset/next. > > > > Hi Philip, > > > > It seems reset/next has not made it to v5.7-rc1 > > > > Would it be possible to provide an immutable branch with this change, or > > maybe let Kevin apply this change through the amlogic tree ? > > > > It would allow us to progress on some DT changes during this new cycle. > > > > Thanks > > Jerome > > Hi Philip, how can we move forward on this ? Sorry for the delay, I have missed the last window. I've now created an immutable branch: git://git.pengutronix.de/pza/linux.git reset/meson-gxl-dac which I will be included in the next reset pull request. regards Philipp
Re: [PATCH v2 22/91] reset: Move reset-simple header out of drivers/reset
On Fri, 2020-04-24 at 17:34 +0200, Maxime Ripard wrote: > The reset-simple code can be useful for drivers outside of drivers/reset > that have a few reset controls as part of their features. Let's move it to > include/linux/reset. > > Cc: Philipp Zabel > Signed-off-by: Maxime Ripard Reviewed-by: Philipp Zabel Do you need the reset patches applied together with this series, or can I pick them up individually? regards Philipp
Re: [PATCH v2 23/91] reset: simple: Add reset callback
Hi Maxime,
On Fri, 2020-04-24 at 17:34 +0200, Maxime Ripard wrote:
> The reset-simple code lacks a reset callback that is still pretty easy to
> implement. The only real thing to consider is the delay needed for a device
> to be reset, so let's expose that as part of the reset-simple driver data.
>
> Cc: Philipp Zabel
> Signed-off-by: Maxime Ripard
> ---
> drivers/reset/reset-simple.c | 24
> include/linux/reset/reset-simple.h | 6 ++
> 2 files changed, 30 insertions(+)
>
> diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
> index c854aa351640..602ed972b0a9 100644
> --- a/drivers/reset/reset-simple.c
> +++ b/drivers/reset/reset-simple.c
> @@ -11,6 +11,7 @@
> * Maxime Ripard
> */
>
> +#include
> #include
> #include
> #include
> @@ -63,6 +64,28 @@ static int reset_simple_deassert(struct
> reset_controller_dev *rcdev,
> return reset_simple_update(rcdev, id, false);
> }
>
> +static int reset_simple_reset(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct reset_simple_data *data = to_reset_simple_data(rcdev);
> + int ret;
> +
> + if (!data->reset_us)
> + return -ENOTSUPP;
> +
> + ret = reset_simple_assert(rcdev, id);
> + if (ret)
> + return ret;
> +
> + usleep_range(data->reset_us, data->reset_us * 2);
> +
> + ret = reset_simple_deassert(rcdev, id);
> + if (ret)
> + return ret;
> +
> + return 0;
Just
return reset_simple_deassert(rcdev, id);
here.
> +}
> +
> static int reset_simple_status(struct reset_controller_dev *rcdev,
> unsigned long id)
> {
> @@ -80,6 +103,7 @@ static int reset_simple_status(struct reset_controller_dev
> *rcdev,
> const struct reset_control_ops reset_simple_ops = {
> .assert = reset_simple_assert,
> .deassert = reset_simple_deassert,
> + .reset = reset_simple_reset,
> .status = reset_simple_status,
> };
> EXPORT_SYMBOL_GPL(reset_simple_ops);
> diff --git a/include/linux/reset/reset-simple.h
> b/include/linux/reset/reset-simple.h
> index 08ccb25a55e6..5eb83625a495 100644
> --- a/include/linux/reset/reset-simple.h
> +++ b/include/linux/reset/reset-simple.h
> @@ -27,6 +27,11 @@
> * @status_active_low: if true, bits read back as cleared while the reset is
> * asserted. Otherwise, bits read back as set while the
> * reset is asserted.
> + * @reset_us: Minimum delay in microseconds needed that needs to be
> + *waited for between an assert and a deassert to reset the
> + *device. If multiple consumers with different delay
> + *requirements are connected to this controller, it must
> + *be the largest minimum delay.
Please mention that 0 does not meen 0 µs delay, but unknown and thus
reset callback not supported.
With these two issues fixed
Reviewed-by: Philipp Zabel
regards
Philipp
[RFC] docs: add a reset controller chapter to the driver API docs
Add initial reset controller API documentation. This is mostly indented to describe the concepts to users of the consumer API, and to tie the kerneldoc comments we already have into the driver API documentation. Signed-off-by: Philipp Zabel --- Documentation/driver-api/index.rst | 1 + Documentation/driver-api/reset.rst | 217 + 2 files changed, 218 insertions(+) create mode 100644 Documentation/driver-api/reset.rst diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst index 38e638abe3eb..0d589829c677 100644 --- a/Documentation/driver-api/index.rst +++ b/Documentation/driver-api/index.rst @@ -29,6 +29,7 @@ available subsections can be seen below. sound frame-buffer regulator + reset iio/index input usb/index diff --git a/Documentation/driver-api/reset.rst b/Documentation/driver-api/reset.rst new file mode 100644 index ..210ccd97c5f0 --- /dev/null +++ b/Documentation/driver-api/reset.rst @@ -0,0 +1,217 @@ +.. SPDX-License-Identifier: GPL-2.0-only + + +Reset controller API + + +Introduction + + +Reset controllers are central units that control the reset signals to multiple +peripherals. +The reset controller API is split in two parts: +the `consumer driver interface <#consumer-driver-interface>`__ (`API reference +<#reset-consumer-api>`__), which allows peripheral drivers to request control +over their reset input signals, and the `reset controller driver interface +<#reset-controller-driver-interface>`__ (`API reference +<#reset-controller-driver-api>`__), which is used by drivers for reset +controller devices to register their reset controls to provide them to the +consumers. + +While some reset controller hardware units also implement system restart +functionality, restart handlers are out of scope for the reset controller API. + +Glossary + + +The reset controller API uses these terms with a specific meaning: + +Reset line + +Physical reset line carrying a reset signal from a reset controller +hardware unit to a peripheral module. + +Reset control + +Control method that determines the state of one or multiple reset lines. +Most commonly this is a single bit in reset controller register space that +either allows direct control over the physical state of the reset line, or +is self-clearing and can be used to trigger a predetermined pulse on the +reset line. +In more complicated reset controls, a single trigger action can launch a +carefully timed sequence of pulses on multiple reset lines. + +Reset controller + +A hardware module that provides a number of reset controls to control a +number of reset lines. + +Reset consumer + +Peripheral module or external IC that is put into reset by the signal on a +reset line. + +Consumer driver interface += + +This interface offers a similar API to the kernel clock framework. +Consumer drivers use get and put operations to acquire and release reset +controls. +Functions are provided to assert and deassert the controlled reset lines, +trigger reset pulses, or to query reset line status. + +When requesting reset controls, consumers can use symbolic names for their +reset inputs, which are mapped to an actual reset control on an existing reset +controller device by the core. + +A stub version of this API is provided when the reset controller framework is +not in use in order to minimise the need to use ifdefs. + +Shared and exclusive resets +--- + +The reset controller API provides either reference counted deassertion and +assertion or direct, exclusive control. +The distinction between shared and exclusive reset controls is made at the time +the reset control is requested, either via :c:func:`devm_reset_control_get_shared` +or via :c:func:`devm_reset_control_get_exclusive`. +This choice determines the behavior of the API calls made with the reset +control. + +Shared resets behave similarly to clocks in the kernel clock framework. +They provide reference counted deassertion, where only the first deassert, +which increments the deassertion reference count to one, and the last assert +which decrements the deassertion reference count back to zero, have a physical +effect on the reset line. + +Exclusive resets on the other hand guarantee direct control. +That is, an assert causes the reset line to be asserted immediately, and a +deassert causes the reset line to be deasserted immediately. + +Assertion and deassertion +- + +Consumer drivers use the :c:func:`reset_control_assert` and +:c:func:`reset_control_deassert` functions to assert and deassert reset lines. +For shared reset controls, calls to the two functions must be balanced. + +Note that since multiple consumers may be using a shared reset control, there +is no guarantee that calling reset_control_assert() o
[PATCH] reset: document (devm_)reset_control_get_optional variants
Add kerneldoc comments for the optional reset_control_get variants.
Signed-off-by: Philipp Zabel
---
include/linux/reset.h | 46 +++
1 file changed, 46 insertions(+)
diff --git a/include/linux/reset.h b/include/linux/reset.h
index eb597e8aa430..df10703351a9 100644
--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -203,12 +203,34 @@ static inline struct reset_control
*reset_control_get_shared(
return __reset_control_get(dev, id, 0, true, false, false);
}
+/**
+ * reset_control_get_optional_exclusive - optional
reset_control_get_exclusive()
+ * @dev: device to be reset by the controller
+ * @id: reset line name
+ *
+ * Optional variant of reset_control_get_exclusive(). If the requested reset
+ * is not specified in the device tree, this function returns NULL instead of
+ * an error.
+ *
+ * See :c:func:`reset_control_get_exclusive` for more information.
+ */
static inline struct reset_control *reset_control_get_optional_exclusive(
struct device *dev, const char *id)
{
return __reset_control_get(dev, id, 0, false, true, true);
}
+/**
+ * reset_control_get_optional_shared - optional reset_control_get_shared()
+ * @dev: device to be reset by the controller
+ * @id: reset line name
+ *
+ * Optional variant of reset_control_get_shared(). If the requested reset
+ * is not specified in the device tree, this function returns NULL instead of
+ * an error.
+ *
+ * See :c:func:`reset_control_get_shared` for more information.
+ */
static inline struct reset_control *reset_control_get_optional_shared(
struct device *dev, const char *id)
{
@@ -354,12 +376,36 @@ static inline struct reset_control
*devm_reset_control_get_shared(
return __devm_reset_control_get(dev, id, 0, true, false, false);
}
+/**
+ * devm_reset_control_get_optional_exclusive - resource managed
+ *
reset_control_get_optional_exclusive()
+ * @dev: device to be reset by the controller
+ * @id: reset line name
+ *
+ * Managed reset_control_get_optional_exclusive(). For reset controllers
+ * returned from this function, reset_control_put() is called automatically on
+ * driver detach.
+ *
+ * See :c:func:`reset_control_get_optional_exclusive` for more information.
+ */
static inline struct reset_control *devm_reset_control_get_optional_exclusive(
struct device *dev, const char *id)
{
return __devm_reset_control_get(dev, id, 0, false, true, true);
}
+/**
+ * devm_reset_control_get_optional_shared - resource managed
+ * reset_control_get_optional_shared()
+ * @dev: device to be reset by the controller
+ * @id: reset line name
+ *
+ * Managed reset_control_get_optional_shared(). For reset controllers returned
+ * from this function, reset_control_put() is called automatically on driver
+ * detach.
+ *
+ * See :c:func:`reset_control_get_optional_shared` for more information.
+ */
static inline struct reset_control *devm_reset_control_get_optional_shared(
struct device *dev, const char *id)
{
--
2.20.1
[PATCH] reset: improve of_xlate documentation
Mention of_reset_simple_xlate as the default if of_xlate is not set.
Signed-off-by: Philipp Zabel
---
drivers/reset/core.c | 6 --
include/linux/reset-controller.h | 3 ++-
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 660e0b07feca..3066f12f70db 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -77,8 +77,10 @@ static const char *rcdev_name(struct reset_controller_dev
*rcdev)
* @rcdev: a pointer to the reset controller device
* @reset_spec: reset line specifier as found in the device tree
*
- * This simple translation function should be used for reset controllers
- * with 1:1 mapping, where reset lines can be indexed by number without gaps.
+ * This static translation function is be used by default if of_xlate in
+ * :c:type:`reset_controller_dev` is not set. It is useful for all reset
+ * controllers with 1:1 mapping, where reset lines can be indexed by number
+ * without gaps.
*/
static int of_reset_simple_xlate(struct reset_controller_dev *rcdev,
const struct of_phandle_args *reset_spec)
diff --git a/include/linux/reset-controller.h b/include/linux/reset-controller.h
index 984f625d5593..c53626c07e87 100644
--- a/include/linux/reset-controller.h
+++ b/include/linux/reset-controller.h
@@ -62,7 +62,8 @@ struct reset_control_lookup {
* @of_node: corresponding device tree node as phandle target
* @of_reset_n_cells: number of cells in reset line specifiers
* @of_xlate: translation function to translate from specifier as found in the
- *device tree to id as given to the reset control ops
+ *device tree to id as given to the reset control ops, defaults
+ *to :c:func:`of_reset_simple_xlate`.
* @nr_resets: number of reset controls in this reset controller device
*/
struct reset_controller_dev {
--
2.20.1
[PATCH] reset: fix reset_control_get_exclusive kerneldoc comment
Add missing parentheses to correctly hyperlink the reference to
reset_control_get_shared().
Fixes: 0b52297f2288 ("reset: Add support for shared reset controls")
Signed-off-by: Philipp Zabel
---
include/linux/reset.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/reset.h b/include/linux/reset.h
index e7793fc0fa93..eb597e8aa430 100644
--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -143,7 +143,7 @@ static inline int device_reset_optional(struct device *dev)
* If this function is called more than once for the same reset_control it will
* return -EBUSY.
*
- * See reset_control_get_shared for details on shared references to
+ * See reset_control_get_shared() for details on shared references to
* reset-controls.
*
* Use of id names is optional.
--
2.20.1
[PATCH] reset: fix reset_control_lookup kerneldoc comment
Add a missing colon to fix a documentation build warning:
./include/linux/reset-controller.h:45: warning: Function parameter or member
'con_id' not described in 'reset_control_lookup'
Fixes: 6691dffab0ab ("reset: add support for non-DT systems")
Signed-off-by: Philipp Zabel
---
include/linux/reset-controller.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/reset-controller.h b/include/linux/reset-controller.h
index 9326d671b6e6..984f625d5593 100644
--- a/include/linux/reset-controller.h
+++ b/include/linux/reset-controller.h
@@ -33,7 +33,7 @@ struct of_phandle_args;
* @provider: name of the reset controller device controlling this reset line
* @index: ID of the reset controller in the reset controller device
* @dev_id: name of the device associated with this reset line
- * @con_id name of the reset line (can be NULL)
+ * @con_id: name of the reset line (can be NULL)
*/
struct reset_control_lookup {
struct list_head list;
--
2.20.1
[PATCH] reset: fix of_reset_control_get_count kerneldoc comment
Add a newline and remove a superfluous kerneldoc marker before the
of_reset_control_get_count kerneldoc comment, to fix documentation
build warnings:
./drivers/reset/core.c:832: warning: Incorrect use of kernel-doc format: *
of_reset_control_get_count - Count number of resets available with a device
./drivers/reset/core.c:840: warning: Function parameter or member 'node' not
described in 'of_reset_control_get_count'
Fixes: 17c82e206d2a ("reset: Add APIs to manage array of resets")
Signed-off-by: Philipp Zabel
---
drivers/reset/core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 7c95cafcdf08..660e0b07feca 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -824,9 +824,10 @@ int __device_reset(struct device *dev, bool optional)
}
EXPORT_SYMBOL_GPL(__device_reset);
-/**
+/*
* APIs to manage an array of reset controls.
*/
+
/**
* of_reset_control_get_count - Count number of resets available with a device
*
--
2.20.1
[PATCH] reset: fix of_reset_simple_xlate kerneldoc comment
The flags parameter never made it into the API, but was erroneously
included in the kerneldoc comment. Remove it to fix a documentation
build warning:
./drivers/reset/core.c:86: warning: Excess function parameter 'flags'
description in 'of_reset_simple_xlate'
Fixes: 61fc41317666 ("reset: Add reset controller API")
Signed-off-by: Philipp Zabel
---
drivers/reset/core.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 213ff40dda11..7c95cafcdf08 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -76,7 +76,6 @@ static const char *rcdev_name(struct reset_controller_dev
*rcdev)
* of_reset_simple_xlate - translate reset_spec to the reset line number
* @rcdev: a pointer to the reset controller device
* @reset_spec: reset line specifier as found in the device tree
- * @flags: a flags pointer to fill in (optional)
*
* This simple translation function should be used for reset controllers
* with 1:1 mapping, where reset lines can be indexed by number without gaps.
--
2.20.1
Re: [PATCH] MAINTAINERS: add reset controller framework keywords
On Tue, 2019-10-22 at 09:03 -0700, Joe Perches wrote: > On Tue, 2019-10-22 at 14:38 +0200, Philipp Zabel wrote: > > Add a regex that matches users of the reset controller API. > [] > > diff --git a/MAINTAINERS b/MAINTAINERS > [] > > @@ -13851,6 +13851,7 @@ F: include/dt-bindings/reset/ > > F: include/linux/reset.h > > F: include/linux/reset/ > > F: include/linux/reset-controller.h > > +K: \b(devm_|of_)?reset_control(ler_[a-z]+|_[a-z_]+)?\b > > Please add ?: to each group so no capture is done. > It's a little faster. > > K:\b(?:devm_|of_)?reset_control(?:ler_[a-z]+|_[a-z_]+)?\b Thank you, fixed in v2. regards Philipp
[PATCH v2] MAINTAINERS: add reset controller framework keywords
Add a regex that matches users of the reset controller API. Signed-off-by: Philipp Zabel --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 296de2b51c83..5d77a376a45c 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -13851,6 +13851,7 @@ F: include/dt-bindings/reset/ F: include/linux/reset.h F: include/linux/reset/ F: include/linux/reset-controller.h +K: \b(?:devm_|of_)?reset_control(?:ler_[a-z]+|_[a-z_]+)?\b RESTARTABLE SEQUENCES SUPPORT M: Mathieu Desnoyers -- 2.20.1
[PATCH] MAINTAINERS: add reset controller framework keywords
Add a regex that matches users of the reset controller API. Signed-off-by: Philipp Zabel --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 296de2b51c83..7ec2f5c48616 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -13851,6 +13851,7 @@ F: include/dt-bindings/reset/ F: include/linux/reset.h F: include/linux/reset/ F: include/linux/reset-controller.h +K: \b(devm_|of_)?reset_control(ler_[a-z]+|_[a-z_]+)?\b RESTARTABLE SEQUENCES SUPPORT M: Mathieu Desnoyers -- 2.20.1
[PATCH] reset: zynqmp: Make reset_control_ops const
The zynqmp_reset_ops structure is never modified. Make it const.
Signed-off-by: Philipp Zabel
---
drivers/reset/reset-zynqmp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/reset/reset-zynqmp.c b/drivers/reset/reset-zynqmp.c
index 99e75d92dada..0144075b11a6 100644
--- a/drivers/reset/reset-zynqmp.c
+++ b/drivers/reset/reset-zynqmp.c
@@ -64,7 +64,7 @@ static int zynqmp_reset_reset(struct reset_controller_dev
*rcdev,
PM_RESET_ACTION_PULSE);
}
-static struct reset_control_ops zynqmp_reset_ops = {
+static const struct reset_control_ops zynqmp_reset_ops = {
.reset = zynqmp_reset_reset,
.assert = zynqmp_reset_assert,
.deassert = zynqmp_reset_deassert,
--
2.20.1
[PATCH] reset: hisilicon: hi3660: Make reset_control_ops const
The hi3660_reset_ops structure is never modified. Make it const.
Signed-off-by: Philipp Zabel
---
drivers/reset/hisilicon/reset-hi3660.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/reset/hisilicon/reset-hi3660.c
b/drivers/reset/hisilicon/reset-hi3660.c
index f690b1878071..a7d4445924e5 100644
--- a/drivers/reset/hisilicon/reset-hi3660.c
+++ b/drivers/reset/hisilicon/reset-hi3660.c
@@ -56,7 +56,7 @@ static int hi3660_reset_dev(struct reset_controller_dev
*rcdev,
return hi3660_reset_deassert(rcdev, idx);
}
-static struct reset_control_ops hi3660_reset_ops = {
+static const struct reset_control_ops hi3660_reset_ops = {
.reset= hi3660_reset_dev,
.assert = hi3660_reset_assert,
.deassert = hi3660_reset_deassert,
--
2.20.1
[PATCH] ARM: prima2: rstc: Make reset_control_ops const
The sirfsoc_rstc_ops structure is never modified. Make it const.
Signed-off-by: Philipp Zabel
---
arch/arm/mach-prima2/rstc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-prima2/rstc.c b/arch/arm/mach-prima2/rstc.c
index 9d56606ac87f..1ee405e2dde9 100644
--- a/arch/arm/mach-prima2/rstc.c
+++ b/arch/arm/mach-prima2/rstc.c
@@ -53,7 +53,7 @@ static int sirfsoc_reset_module(struct reset_controller_dev
*rcdev,
return 0;
}
-static struct reset_control_ops sirfsoc_rstc_ops = {
+static const struct reset_control_ops sirfsoc_rstc_ops = {
.reset = sirfsoc_reset_module,
};
--
2.20.1
Re: [PATCHv2] reset: build simple reset controller driver for Agilex
On Mon, 2019-10-14 at 10:18 -0500, Dinh Nguyen wrote: > The Intel SoCFPGA Agilex platform shares the same reset controller that > is on the Stratix10. > > Signed-off-by: Dinh Nguyen > --- > v2: rebase to v5.4-rc1 > --- > drivers/reset/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig > index 7b07281aa0ae..46f7986c3587 100644 > --- a/drivers/reset/Kconfig > +++ b/drivers/reset/Kconfig > @@ -129,7 +129,7 @@ config RESET_SCMI > > config RESET_SIMPLE > bool "Simple Reset Controller Driver" if COMPILE_TEST > - default ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI || ARCH_ZX || > ARCH_ASPEED || ARCH_BITMAIN || ARC > + default ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI || ARCH_ZX || > ARCH_ASPEED || ARCH_BITMAIN || ARC || ARCH_AGILEX > help > This enables a simple reset controller driver for reset lines that > that can be asserted and deasserted by toggling bits in a contiguous, Thank you, applied to reset/next. regards Philipp
Re: [PATCH] reset: Fix memory leak in reset_control_array_put()
Hi Kishon, On Tue, 2019-10-22 at 14:06 +0530, Kishon Vijay Abraham I wrote: > Memory allocated for 'struct reset_control_array' in > of_reset_control_array_get() is never freed in > reset_control_array_put() resulting in kmemleak showing > the following backtrace. > > backtrace: > [] __kmalloc+0x1b0/0x2b0 > [ ] of_reset_control_array_get+0xa4/0x180 > [<4cc02754>] 0x88c669e4 > [<50a83b24>] platform_drv_probe+0x50/0xa0 > [ ] really_probe+0x108/0x348 > [<5aa458ac>] driver_probe_device+0x58/0x100 > [<8853626c>] device_driver_attach+0x6c/0x90 > [<85308d19>] __driver_attach+0x84/0xc8 > [<080d35f2>] bus_for_each_dev+0x74/0xc8 > [ ] driver_attach+0x20/0x28 > [<923ba6e6>] bus_add_driver+0x148/0x1f0 > [<61473b66>] driver_register+0x60/0x110 > [ ] __platform_driver_register+0x40/0x48 > [<7c764b4f>] 0x88c6c020 > [<47ec2e8c>] do_one_initcall+0x5c/0x1b0 > [<93d4b50d>] do_init_module+0x54/0x1d0 > > Signed-off-by: Kishon Vijay Abraham I > --- > drivers/reset/core.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/reset/core.c b/drivers/reset/core.c > index 213ff40dda11..36b1ff69b1e2 100644 > --- a/drivers/reset/core.c > +++ b/drivers/reset/core.c > @@ -748,6 +748,7 @@ static void reset_control_array_put(struct > reset_control_array *resets) > for (i = 0; i < resets->num_rstcs; i++) > __reset_control_put_internal(resets->rstc[i]); > mutex_unlock(_list_mutex); > + kfree(resets); > } > > /** Thank you for the patch! I've added a Fixes: 17c82e206d2a ("reset: Add APIs to manage array of resets") tag and applied it to my reset/fixes branch. regards Philipp
Re: [PATCH] reset: hi6220: Add support for AO reset controller
Hi Peter,
On Wed, 2019-10-09 at 17:20 +0100, Peter Griffin wrote:
[...]
> > Do you know whether this is actually a module reset going to the GPU,
> > or if this is somehow part of the clock and power gating machinery?
>
> I don't know for sure, there is no documentation for these registers
> apart from the code in [1] and [2], and that is really just the register name.
Ok.
> > There's also clock and isolation cell control going on, so this looks a
> > bit like it should be part of a power domain driver.
>
> I choose to add it here as this driver was already managing the
> MEDIA_SCTRL_SC_MEDIA_RSTDIS_ADDR register, so it seemed correct for it to also
> manage the AO_SCTRL_SC_PW_RSTDIS0_ADDR register.
I agree here ...
> The write to AO_SCTRL_SC_PW_ISODIS0_ADDR and
> AO_SCTRL_SC_PW_CLKEN0_ADDR I guess aren't so clear cut but I wanted to
> maintain the
> same ordering from [1].
... but not here, necessarily. These register names make it look like
this is the wrong place.
On the other hand, if you don't actually have a way to disable power to
the GPU domain, it is a bit hard for me to argue that it would be more
correct to place this in a power domain driver. Certainly the power
domain driver could (de)assert the reset in the right order though.
I notice the actual GPU reset seems to be MEDIA_G3D, and is deasserted
after whatever is controlled via AO_SCTRL_SC_PW_* in [1], so maybe this
is indeed just the power domain control?
> I have no info about the power domains on the SoC, so not sure it will be
> possible to make it part of a power domain driver.
>
> > Do you have an (internal or external) regulator that could be disabled
> > while the GPU is inactive, like [1]?
> >
> > [1]
> > https://raw.githubusercontent.com/96boards/meta-96boards/master/recipes-kernel/linux/linux-96boards/0004-drivers-gpu-arm-utgard-add-basic-HiKey-platform-file.patch
>
> This patch is based off [1] the regulator mentioned there G3D_PD_VDD it turns
> out has never been available and always fails to do anything.
>
> So AFAIK there is no external regulator available to disable. This code has
> since been
> removed from Johns tree [2].
>
> [2]
> https://git.linaro.org/people/john.stultz/android-dev.git/commit/?h=dev/hikey-5.3=64ec445a8271a2ced841484492ed9bf2e1ef4313
The comment
"Note: GPU DVFS is not implemented and a custom Device Tree
entry is needed by these platform files. This is a
first working version that needs to be improved."
doesn't exactly make it clear whether it is "not implemented" in the
SoC, not implemented on the board, or just not implemented in the driver
in this patchset.
So even if this board has the regulator fixed, could there be others
where GPU power could be switched off? I wouldn't want to encode the the
limitations of a particular board into the SoC DT bindings.
> > > Cc: Philipp Zabel
> > > Cc: Peter Griffin
> > > Cc: Enrico Weigelt
> > > Signed-off-by: Peter Griffin
> > > Signed-off-by: John Stultz
> > > ---
> > > drivers/reset/hisilicon/hi6220_reset.c | 51 +-
> > > 1 file changed, 50 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/reset/hisilicon/hi6220_reset.c
> > > b/drivers/reset/hisilicon/hi6220_reset.c
> > > index 24e6d420b26b..d84674a2cead 100644
> > > --- a/drivers/reset/hisilicon/hi6220_reset.c
> > > +++ b/drivers/reset/hisilicon/hi6220_reset.c
> > > @@ -33,6 +33,7 @@
> > > enum hi6220_reset_ctrl_type {
> > > PERIPHERAL,
> > > MEDIA,
> > > + AO,
> > > };
> > >
> > > struct hi6220_reset_data {
> > > @@ -92,6 +93,47 @@ static const struct reset_control_ops
> > > hi6220_media_reset_ops = {
> > > .deassert = hi6220_media_deassert,
> > > };
> > >
> > > +#define AO_SCTRL_SC_PW_CLKEN0 0x800
> > > +#define AO_SCTRL_SC_PW_CLKDIS00x804
> > > +
> > > +#define AO_SCTRL_SC_PW_RSTEN0 0x810
> > > +#define AO_SCTRL_SC_PW_RSTDIS00x814
Is this the only reset register in the "Always-On" (that would have been
nice to include in the commit description for my education) controller?
According to [3] there also seem to be rst4 and rst5 registers?
[3]
https://github.com/u-boot/u-boot/blob/master/arch/arm/include/asm/arch-hi6220/hi6220_regs_alwayson.h
> > > +
> > > +#define AO_SCTRL_SC_PW_ISOEN0 0x820
> > > +#define AO_SCTRL_SC_PW_ISODIS00x824
> > > +#define AO_MAX_INDEX 12
Where does this number come from? I can't find any information about the
bits in the PW_* registers...
> > > +
>
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin, On Mon, 2019-10-07 at 21:53 +0200, Martin Blumenstingl wrote: > Hi Philipp, > > On Thu, Oct 3, 2019 at 4:19 PM Philipp Zabel wrote: > [...] > > > because the register layout was greatly simplified for the newer SoCs > > > (for which there is reset-intel) compared to the older ones > > > (reset-lantiq). > > > Dilip's suggestion (in my own words) is that you take his new > > > reset-intel driver, then we will work on porting reset-lantiq over to > > > that so in the end we can drop the reset-lantiq driver. > > > > Just to be sure, you are suggesting to add support for the current > > lantiq,reset binding to the reset-intel driver at a later point? I > > see no reason not to do that, but I'm also not quite sure what the > > benefit will be over just keeping reset-lantiq as is? > > according to Chuan and Dilip the current reset-lantiq implementation > is wrong [0]. The only issue seems to be the .reset callback, which doesn't have any users anway. > my understanding is that the Lantiq and Intel LGM reset controllers > are identical except: > - the Lantiq variant uses a weird register layout (reset and status > registers not at consecutive offsets) > - the bits of the reset and status registers sometimes don't match on > the Lantiq variant Thank you, so these are a good explanation for why the DT bindings should be different. > - the Intel variant has a dedicated registers area for the reset > controller registers, while the Lantiq variant mixes them with various > other functionality (for example: USB2 PHYs) I'm not quite sure I understand why the intel driver is using syscon, then. Either way, it shouldn't make a big difference if regmap is used anyway. > > > This approach means more work for me (as I am probably the one who > > > then has to do the work to port reset-lantiq over to reset-intel). > > > > More work than what alternative? > > compared to "fixing" the existing reset-lantiq driver (reset callback) That is still something you could do, or just drop the .reset callback because there are no reset consumers using it anyway. One correct thing to do would be to identify those self-clearing reset bits and to disallow calling assert/deassert on them. > and then (instead of adding a new driver) integrating Intel LGM > support into reset-lantiq Since at this point I'm not even sure whether merging the two at all is better than keeping them separate, I have no opinion on whether merging intel support into the lantiq driver or the other way around is preferable. > > > I'm happy to do that work if you think that it's worth following this > > > approach. So I want your opinion on this before I spend any effort on > > > porting reset-lantiq over to reset-intel. > > > > Reset drivers are typically so simple, I'm not quite sure whether it is > > worth to integrate multiple drivers if it complicates matters too much. > > In this case though I expect it would just be adding support for a > > custom .of_xlate and lantiq specific register property parsing? > > yes, that's how I understand the Lantiq and Intel reset controllers: > - reset/status/assert/deassert callbacks would be shared across all variants > - register parsing and of_xlate are SoC specific Ok. If that turns out to be less rather than more boilerplate than two separate drivers, that should be fine. regards Philipp
Re: [PATCH v3 0/3] reset: meson: add Meson-A1 SoC support
Hi Xingyu, On Sun, 2019-09-29 at 14:24 +0800, Xingyu Chen wrote: > This patchset adds support for Meson-A1 SoC Reset Controller. A new struct > meson_reset_param is introduced to describe the register differences between > Meson-A1 and previous SoCs. > > Changes since v2 at [1]: > - add comments in header file to indicate holes > - reorder the Signed-off-by and Reviewed-by > - remove Jianxin's Signed-off-by > - add Kevin's Reviewed-by Thank you, I have applied patches 2 and 3 to reset/next. regards Philipp > Changes since v1 at [0]: > - rebase on linux-next > - add Neil's Reviewed-by > > [0] > https://lore.kernel.org/linux-amlogic/1568808746-1153-1-git-send-email-xingyu.c...@amlogic.com > [1] > https://lore.kernel.org/linux-amlogic/1569227661-4261-1-git-send-email-xingyu.c...@amlogic.com > > Xingyu Chen (3): > arm64: dts: meson: add reset controller for Meson-A1 SoC > dt-bindings: reset: add bindings for the Meson-A1 SoC Reset Controller > reset: add support for the Meson-A1 SoC Reset Controller > > .../bindings/reset/amlogic,meson-reset.yaml| 1 + > arch/arm64/boot/dts/amlogic/meson-a1.dtsi | 6 ++ > drivers/reset/reset-meson.c| 35 -- > include/dt-bindings/reset/amlogic,meson-a1-reset.h | 74 > ++ > 4 files changed, 109 insertions(+), 7 deletions(-) > create mode 100644 include/dt-bindings/reset/amlogic,meson-a1-reset.h >
Re: [PATCH] reset: hi6220: Add support for AO reset controller
Hi John, Peter,
On Tue, 2019-10-01 at 18:21 +, John Stultz wrote:
> From: Peter Griffin
>
> This is required to bring Mali450 gpu out of reset.
Do you know whether this is actually a module reset going to the GPU,
or if this is somehow part of the clock and power gating machinery?
There's also clock and isolation cell control going on, so this looks a
bit like it should be part of a power domain driver.
Do you have an (internal or external) regulator that could be disabled
while the GPU is inactive, like [1]?
[1]
https://raw.githubusercontent.com/96boards/meta-96boards/master/recipes-kernel/linux/linux-96boards/0004-drivers-gpu-arm-utgard-add-basic-HiKey-platform-file.patch
> Cc: Philipp Zabel
> Cc: Peter Griffin
> Cc: Enrico Weigelt
> Signed-off-by: Peter Griffin
> Signed-off-by: John Stultz
> ---
> drivers/reset/hisilicon/hi6220_reset.c | 51 +-
> 1 file changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/reset/hisilicon/hi6220_reset.c
> b/drivers/reset/hisilicon/hi6220_reset.c
> index 24e6d420b26b..d84674a2cead 100644
> --- a/drivers/reset/hisilicon/hi6220_reset.c
> +++ b/drivers/reset/hisilicon/hi6220_reset.c
> @@ -33,6 +33,7 @@
> enum hi6220_reset_ctrl_type {
> PERIPHERAL,
> MEDIA,
> + AO,
> };
>
> struct hi6220_reset_data {
> @@ -92,6 +93,47 @@ static const struct reset_control_ops
> hi6220_media_reset_ops = {
> .deassert = hi6220_media_deassert,
> };
>
> +#define AO_SCTRL_SC_PW_CLKEN0 0x800
> +#define AO_SCTRL_SC_PW_CLKDIS00x804
> +
> +#define AO_SCTRL_SC_PW_RSTEN0 0x810
> +#define AO_SCTRL_SC_PW_RSTDIS00x814
> +
> +#define AO_SCTRL_SC_PW_ISOEN0 0x820
> +#define AO_SCTRL_SC_PW_ISODIS00x824
> +#define AO_MAX_INDEX 12
> +
> +static int hi6220_ao_assert(struct reset_controller_dev *rc_dev,
> + unsigned long idx)
> +{
> + struct hi6220_reset_data *data = to_reset_data(rc_dev);
> + struct regmap *regmap = data->regmap;
> + int ret;
> +
> + ret = regmap_write(regmap, AO_SCTRL_SC_PW_RSTEN0, BIT(idx));
> + ret |= regmap_write(regmap, AO_SCTRL_SC_PW_ISOEN0, BIT(idx));
> + ret |= regmap_write(regmap, AO_SCTRL_SC_PW_CLKDIS0, BIT(idx));
I would like the return values not to be ORed together, since they are
returned to the caller.
> + return ret;
> +}
> +
> +static int hi6220_ao_deassert(struct reset_controller_dev *rc_dev,
> + unsigned long idx)
> +{
> + struct hi6220_reset_data *data = to_reset_data(rc_dev);
> + struct regmap *regmap = data->regmap;
> + int ret;
> +
> + ret = regmap_write(regmap, AO_SCTRL_SC_PW_RSTDIS0, BIT(idx));
> + ret |= regmap_write(regmap, AO_SCTRL_SC_PW_ISODIS0, BIT(idx));
> + ret |= regmap_write(regmap, AO_SCTRL_SC_PW_CLKEN0, BIT(idx));
Are you sure these are fire and forget, or might the order be important?
It might be necessary to disable isolation before enabling the clocks
and deasserting reset, to avoid glitches.
> + return ret;
> +}
> +
> +static const struct reset_control_ops hi6220_ao_reset_ops = {
> + .assert = hi6220_ao_assert,
> + .deassert = hi6220_ao_deassert,
> +};
> +
> static int hi6220_reset_probe(struct platform_device *pdev)
> {
> struct device_node *np = pdev->dev.of_node;
> @@ -117,9 +159,12 @@ static int hi6220_reset_probe(struct platform_device
> *pdev)
> if (type == MEDIA) {
> data->rc_dev.ops = _media_reset_ops;
> data->rc_dev.nr_resets = MEDIA_MAX_INDEX;
> - } else {
> + } else if (type == PERIPHERAL) {
> data->rc_dev.ops = _peripheral_reset_ops;
> data->rc_dev.nr_resets = PERIPH_MAX_INDEX;
> + } else {
> + data->rc_dev.ops = _ao_reset_ops;
> + data->rc_dev.nr_resets = AO_MAX_INDEX;
> }
>
> return reset_controller_register(>rc_dev);
> @@ -134,6 +179,10 @@ static const struct of_device_id hi6220_reset_match[] = {
> .compatible = "hisilicon,hi6220-mediactrl",
> .data = (void *)MEDIA,
> },
> + {
> + .compatible = "hisilicon,hi6220-aoctrl",
> + .data = (void *)AO,
> + },
> { /* sentinel */ },
> };
> MODULE_DEVICE_TABLE(of, hi6220_reset_match);
regards
Philipp
Re: [PATCH v2 0/2] reset: meson-audio-arb: add sm1 support
Hi Jerome, On Tue, Oct 01, 2019 at 11:40:20AM +0200, Jerome Brunet wrote: [...] > Looks like this patchset missed v5.4-rc1. > Could you provide a tag with the bindings to Kevin so we can use the IDs > in DT until the next merge window ? Does git://git.pengutronix.de/git/pza/linux.git reset/meson-sm1-bindings work for you? regards Philipp
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin, Dilip,
On Thu, Sep 19, 2019 at 09:51:48PM +0200, Martin Blumenstingl wrote:
> Hi Dilip,
>
> (sorry for the late reply)
Same, sorry for the delay.
> On Thu, Sep 12, 2019 at 8:38 AM Dilip Kota
> wrote:
> [...]
> > The major difference between the vrx200 and lgm is:
> > 1.) RCU in vrx200 is having multiple register regions wheres RCU in lgm
> > has one single register region.
> > 2.) Register offsets and bit offsets are different.
> >
> > So enhancing the intel-reset-syscon.c to provide compatibility/support
> > for vrx200.
> > Please check the below dtsi binding proposal and let me know your view.
> >
> > rcu0:reset-controller@ {
> > compatible= "intel,rcu-lgm";
> > reg = <0x000 0x8>, , ,
> > ;
> I'm not sure that I understand what are reg_set2/3/4 for
> the first resource (0x8 at 0x0) already covers the whole LGM RCU,
> so what is the purpose of the other register resources
>
> > intel,global-reset = <0x10 30>;
> > #reset-cells = <3>;
> > };
> >
> > "#reset-cells":
> >const:3
> >description: |
> > The 1st cell is the reset register offset.
> > The 2nd cell is the reset set bit offset.
> > The 3rd cell is the reset status bit offset.
> I think this will work fine for VRX200 (and even older SoCs)
> as you have described in your previous emails we can determine the
> status offset from the reset offset using a simple if/else
>
> for LGM I like your initial suggestion with #reset-cells = <2> because
> it's easier to read and write.
>
> > Reset driver takes care of parsing the register address "reg" as per the
> > ".data" structure in struct of_device_id.
> > Reset driver takes care of traversing the status register offset.
> the differentiation between two and three #reset-cells can also happen
> based on the struct of_device_id:
> - the LGM implementation would simply also use the reset bit as status
> bit (only two cells are needed)
> - the implementation for earlier SoCs would parse the third cell and
> use that as status bit
>
> Philipp, can you please share your opinion on how to move forward with
> the reset-intel driver from this series?
That all sounds reasonable for VRX200/LGM to me.
> because the register layout was greatly simplified for the newer SoCs
> (for which there is reset-intel) compared to the older ones
> (reset-lantiq).
> Dilip's suggestion (in my own words) is that you take his new
> reset-intel driver, then we will work on porting reset-lantiq over to
> that so in the end we can drop the reset-lantiq driver.
Just to be sure, you are suggesting to add support for the current
lantiq,reset binding to the reset-intel driver at a later point? I
see no reason not to do that, but I'm also not quite sure what the
benefit will be over just keeping reset-lantiq as is?
> This approach means more work for me (as I am probably the one who
> then has to do the work to port reset-lantiq over to reset-intel).
More work than what alternative?
> I'm happy to do that work if you think that it's worth following this
> approach. So I want your opinion on this before I spend any effort on
> porting reset-lantiq over to reset-intel.
Reset drivers are typically so simple, I'm not quite sure whether it is
worth to integrate multiple drivers if it complicates matters too much.
In this case though I expect it would just be adding support for a
custom .of_xlate and lantiq specific register property parsing?
regards
Philipp
Re: [PATCH] reset: uniphier-glue: Add Pro5 USB3 support
Hi Kunihiko, On Tue, 2019-09-10 at 20:56 +0900, Kunihiko Hayashi wrote: [...] > This driver is derived from reset-simple, so the method to control reset > in the glue block is the same for each SoC. > > And both Pro4 and Pro5 need same parent clock and reset, so the data for > these SoCs refer same parent clock names and parent reset names. > > However, since the glue block itself can be different, I think that > compatible string should be distinguished for each SoC. Ok, in that case I'll apply the patch as-is. Thank you for clarifying. regards Philipp
Re: [PATCH] reset: uniphier-glue: Add Pro5 USB3 support
Hi Kunihiko,
On Tue, 2019-09-10 at 10:55 +0900, Kunihiko Hayashi wrote:
> Pro5 SoC has same scheme of USB3 reset as Pro4, so the data for Pro5 is
> equivalent to Pro4.
>
> Signed-off-by: Kunihiko Hayashi
If it is exactly the same, you could keep using the same compatible:
> ---
> Documentation/devicetree/bindings/reset/uniphier-reset.txt | 5 +++--
> drivers/reset/reset-uniphier-glue.c| 4
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> b/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> index ea00517..e320a8c 100644
> --- a/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> +++ b/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> @@ -130,6 +130,7 @@ this layer. These clocks and resets should be described
> in each property.
> Required properties:
> - compatible: Should be
> "socionext,uniphier-pro4-usb3-reset" - for Pro4 SoC USB3
> +"socionext,uniphier-pro5-usb3-reset" - for Pro5 SoC USB3
+"socionext,uniphier-pro5-usb3-reset", "socionext,uniphier-pro4-usb3-reset"
- for Pro5 SoC USB3
[...]
> diff --git a/drivers/reset/reset-uniphier-glue.c
> b/drivers/reset/reset-uniphier-glue.c
> index a45923f..2b188b3bb 100644
> --- a/drivers/reset/reset-uniphier-glue.c
> +++ b/drivers/reset/reset-uniphier-glue.c
> @@ -141,6 +141,10 @@ static const struct of_device_id
> uniphier_glue_reset_match[] = {
> .data = _pro4_data,
> },
> {
> + .compatible = "socionext,uniphier-pro5-usb3-reset",
> + .data = _pro4_data,
> + },
> + {
> .compatible = "socionext,uniphier-pxs2-usb3-reset",
> .data = _pxs2_data,
> },
And this change would not be necessary.
regards
Philipp
Re: [PATCH -next] reset: reset-scmi: add missing handle initialisation
Hi Sudeep,
On Mon, 2019-09-09 at 16:21 +0100, Sudeep Holla wrote:
> scmi_reset_data->handle needs to be initialised at probe, so that it
> can be used to access scmi reset protocol apis using the same later.
>
> Since it was tested with a module that obtained handle elsewhere,
> it was missed easily. Add the missing scmi_reset_data->handle
> initialisation to fix the issue.
>
> Fixes: c8ae9c2da1cc ("reset: Add support for resets provided by SCMI")
> Cc: Philipp Zabel
> Reported-by: Etienne Carriere
> Signed-off-by: Sudeep Holla
> ---
> drivers/reset/reset-scmi.c | 1 +
> 1 file changed, 1 insertion(+)
>
> Hi Philipp,
>
> I can either take this via ARM SoC as the driver is getting merged through
> ARM SoC tree, or you can apply this once it gets landed in mainline.
> I am fine with whatever you prefer.
Feel free to take this in via ARM SoC directly, I see no need to wait
for this to hit mainline.
Acked-by: Philipp Zabel
regards
Philipp
Re: [PATCH for 5.4] media: hantro: Fix s_fmt for dynamic resolution changes
On Thu, 2019-09-05 at 15:00 -0300, Ezequiel Garcia wrote:
> On Wed, 2019-09-04 at 15:28 +0200, Philipp Zabel wrote:
> > On Wed, 2019-09-04 at 10:01 -0300, Ezequiel Garcia wrote:
> > > On Wed, 2019-09-04 at 12:13 +0200, Philipp Zabel wrote:
> > > > Hi Ezequiel,
> > > >
> > > > On Tue, 2019-09-03 at 14:12 -0300, Ezequiel Garcia wrote:
> > > > > Commit 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support
> > > > > decoders")
> > > > > changed the conditions under S_FMT was allowed for OUTPUT
> > > > > CAPTURE buffers.
> > > > >
> > > > > However, and according to the mem-to-mem stateless decoder
> > > > > specification,
> > > > > in order to support dynamic resolution changes, S_FMT should be
> > > > > allowed
> > > > > even if OUTPUT buffers have been allocated.
> > > > >
> > > > > Relax decoder S_FMT restrictions on OUTPUT buffers, allowing a
> > > > > resolution
> > > > > modification, provided the pixel format stays the same.
> > > > >
> > > > > Tested on RK3288 platforms using ChromiumOS Video Decode/Encode
> > > > > Accelerator Unittests.
> > > > >
> > > > > Fixes: 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support
> > > > > decoders")
> > > > > Signed-off-by: Ezequiel Garcia
> > > > > ---
> > > > > drivers/staging/media/hantro/hantro_v4l2.c | 22
> > > > > --
> > > > > 1 file changed, 16 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/drivers/staging/media/hantro/hantro_v4l2.c
> > > > > b/drivers/staging/media/hantro/hantro_v4l2.c
> > > > > index 3dae52abb96c..d48b548842cf 100644
> > > > > --- a/drivers/staging/media/hantro/hantro_v4l2.c
> > > > > +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> > > > > @@ -367,19 +367,22 @@ vidioc_s_fmt_out_mplane(struct file *file, void
> > > > > *priv, struct v4l2_format *f)
> > > > > {
> > > > > struct v4l2_pix_format_mplane *pix_mp = >fmt.pix_mp;
> > > > > struct hantro_ctx *ctx = fh_to_ctx(priv);
> > > > > + struct vb2_queue *vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> > > > > f->type);
> > > > > const struct hantro_fmt *formats;
> > > > > unsigned int num_fmts;
> > > > > - struct vb2_queue *vq;
> > > > > int ret;
> > > > >
> > > > > - /* Change not allowed if queue is busy. */
> > > > > - vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> > > > > - if (vb2_is_busy(vq))
> > > > > - return -EBUSY;
> > > > > -
> > > > > if (!hantro_is_encoder_ctx(ctx)) {
> > > > > struct vb2_queue *peer_vq;
> > > > >
> > > > > + /*
> > > > > + * In other to support dynamic resolution change,
> > > > > + * the decoder admits a resolution change, as long
> > > > > + * as the pixelformat remains. Can't be done if
> > > > > streaming.
> > > > > + */
> > > > > + if (vb2_is_streaming(vq) || (vb2_is_busy(vq) &&
> > > > > + pix_mp->pixelformat != ctx->src_fmt.pixelformat))
> > > >
> > > > Before using contents of the v4l2_format f for comparison, we should run
> > > > vidioc_try_fmt_out_mplane over it.
> > >
> > > Right, good catch.
> > >
> > > > Also, besides pixelformat, sizeimage
> > > > shouldn't change either, at least if this is a VB2_MMAP queue.
> > > >
> > >
> > > This is the OUTPUT queue, so I don't see why the sizeimage
> > > of the coded buffers should stay the same. Maybe I'm missing
> > > something?
> >
> > If the OUTPUT vb2_queue is busy, we already have some buffers of the old
> > size allocated. We can't change their size dynamically with just
> > VIDIOC_S_FMT.
> >
> > Maybe this should correct sizeimage to the old size instead of returning
> > -EBUSY? Either way, if the old buffer size is too small to reasonably
> > decode the new resolution, the OUTPUT buffers have to be reallocated.
> >
>
> Note that for a decoder, the OUTPUT side buffers are coded. Is there any
> straightforward correlation between the buffer size and the resolution?
>
> In other words, how does the driver figure out if the size is
> resonably large?
The decoded frame size seems like a reasonable upper bound at first, but
especially for formats like H.264 it is trivial to construct slices that
are larger than that with I_PCM macroblocks.
I think for H.264 some limit depending on profile and level can be
constructed from spec chapter A.3. "Levels", by looking at the rules for
the sum of NumBytesInNALunit, though not quite straightforward.
regards
Philipp
Re: [PATCH v2 0/2] reset: meson-audio-arb: add sm1 support
Hi Jerome, On Thu, 2019-09-05 at 15:50 +0200, Jerome Brunet wrote: > This patchset adds the new arb reset lines for the sm1 SoC family > It has been tested on the sei610 platform. > > Changes since v1 [0]: > * Fix the mistake on the number of reset as reported by Phililpp (thx) > > [0]: https://lkml.kernel.org/r/20190820094625.13455-1-jbru...@baylibre.com > > Jerome Brunet (2): > reset: dt-bindings: meson: update arb bindings for sm1 > reset: meson-audio-arb: add sm1 support > > .../reset/amlogic,meson-axg-audio-arb.txt | 3 +- > drivers/reset/reset-meson-audio-arb.c | 43 +-- > .../reset/amlogic,meson-axg-audio-arb.h | 2 + > 3 files changed, 44 insertions(+), 4 deletions(-) Thank you, both applied to reset/next. regards Philipp
Re: [PATCH for 5.4] media: hantro: Fix s_fmt for dynamic resolution changes
On Wed, 2019-09-04 at 10:01 -0300, Ezequiel Garcia wrote:
> On Wed, 2019-09-04 at 12:13 +0200, Philipp Zabel wrote:
> > Hi Ezequiel,
> >
> > On Tue, 2019-09-03 at 14:12 -0300, Ezequiel Garcia wrote:
> > > Commit 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support
> > > decoders")
> > > changed the conditions under S_FMT was allowed for OUTPUT
> > > CAPTURE buffers.
> > >
> > > However, and according to the mem-to-mem stateless decoder specification,
> > > in order to support dynamic resolution changes, S_FMT should be allowed
> > > even if OUTPUT buffers have been allocated.
> > >
> > > Relax decoder S_FMT restrictions on OUTPUT buffers, allowing a resolution
> > > modification, provided the pixel format stays the same.
> > >
> > > Tested on RK3288 platforms using ChromiumOS Video Decode/Encode
> > > Accelerator Unittests.
> > >
> > > Fixes: 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support
> > > decoders")
> > > Signed-off-by: Ezequiel Garcia
> > > ---
> > > drivers/staging/media/hantro/hantro_v4l2.c | 22 --
> > > 1 file changed, 16 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/hantro/hantro_v4l2.c
> > > b/drivers/staging/media/hantro/hantro_v4l2.c
> > > index 3dae52abb96c..d48b548842cf 100644
> > > --- a/drivers/staging/media/hantro/hantro_v4l2.c
> > > +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> > > @@ -367,19 +367,22 @@ vidioc_s_fmt_out_mplane(struct file *file, void
> > > *priv, struct v4l2_format *f)
> > > {
> > > struct v4l2_pix_format_mplane *pix_mp = >fmt.pix_mp;
> > > struct hantro_ctx *ctx = fh_to_ctx(priv);
> > > + struct vb2_queue *vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> > > const struct hantro_fmt *formats;
> > > unsigned int num_fmts;
> > > - struct vb2_queue *vq;
> > > int ret;
> > >
> > > - /* Change not allowed if queue is busy. */
> > > - vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> > > - if (vb2_is_busy(vq))
> > > - return -EBUSY;
> > > -
> > > if (!hantro_is_encoder_ctx(ctx)) {
> > > struct vb2_queue *peer_vq;
> > >
> > > + /*
> > > + * In other to support dynamic resolution change,
> > > + * the decoder admits a resolution change, as long
> > > + * as the pixelformat remains. Can't be done if streaming.
> > > + */
> > > + if (vb2_is_streaming(vq) || (vb2_is_busy(vq) &&
> > > + pix_mp->pixelformat != ctx->src_fmt.pixelformat))
> >
> > Before using contents of the v4l2_format f for comparison, we should run
> > vidioc_try_fmt_out_mplane over it.
>
> Right, good catch.
>
> > Also, besides pixelformat, sizeimage
> > shouldn't change either, at least if this is a VB2_MMAP queue.
> >
>
> This is the OUTPUT queue, so I don't see why the sizeimage
> of the coded buffers should stay the same. Maybe I'm missing
> something?
If the OUTPUT vb2_queue is busy, we already have some buffers of the old
size allocated. We can't change their size dynamically with just
VIDIOC_S_FMT.
Maybe this should correct sizeimage to the old size instead of returning
-EBUSY? Either way, if the old buffer size is too small to reasonably
decode the new resolution, the OUTPUT buffers have to be reallocated.
regards
Philipp
Re: [PATCH for 5.4] media: hantro: Fix s_fmt for dynamic resolution changes
Hi Ezequiel,
On Tue, 2019-09-03 at 14:12 -0300, Ezequiel Garcia wrote:
> Commit 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support
> decoders")
> changed the conditions under S_FMT was allowed for OUTPUT
> CAPTURE buffers.
>
> However, and according to the mem-to-mem stateless decoder specification,
> in order to support dynamic resolution changes, S_FMT should be allowed
> even if OUTPUT buffers have been allocated.
>
> Relax decoder S_FMT restrictions on OUTPUT buffers, allowing a resolution
> modification, provided the pixel format stays the same.
>
> Tested on RK3288 platforms using ChromiumOS Video Decode/Encode Accelerator
> Unittests.
>
> Fixes: 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support
> decoders")
> Signed-off-by: Ezequiel Garcia
> ---
> drivers/staging/media/hantro/hantro_v4l2.c | 22 --
> 1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/media/hantro/hantro_v4l2.c
> b/drivers/staging/media/hantro/hantro_v4l2.c
> index 3dae52abb96c..d48b548842cf 100644
> --- a/drivers/staging/media/hantro/hantro_v4l2.c
> +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> @@ -367,19 +367,22 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv,
> struct v4l2_format *f)
> {
> struct v4l2_pix_format_mplane *pix_mp = >fmt.pix_mp;
> struct hantro_ctx *ctx = fh_to_ctx(priv);
> + struct vb2_queue *vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> const struct hantro_fmt *formats;
> unsigned int num_fmts;
> - struct vb2_queue *vq;
> int ret;
>
> - /* Change not allowed if queue is busy. */
> - vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> - if (vb2_is_busy(vq))
> - return -EBUSY;
> -
> if (!hantro_is_encoder_ctx(ctx)) {
> struct vb2_queue *peer_vq;
>
> + /*
> + * In other to support dynamic resolution change,
> + * the decoder admits a resolution change, as long
> + * as the pixelformat remains. Can't be done if streaming.
> + */
> + if (vb2_is_streaming(vq) || (vb2_is_busy(vq) &&
> + pix_mp->pixelformat != ctx->src_fmt.pixelformat))
Before using contents of the v4l2_format f for comparison, we should run
vidioc_try_fmt_out_mplane over it. Also, besides pixelformat, sizeimage
shouldn't change either, at least if this is a VB2_MMAP queue.
> + return -EBUSY;
> /*
>* Since format change on the OUTPUT queue will reset
>* the CAPTURE queue, we can't allow doing so
> @@ -389,6 +392,13 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv,
> struct v4l2_format *f)
> V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> if (vb2_is_busy(peer_vq))
> return -EBUSY;
> + } else {
> + /*
> + * The encoder doesn't admit a format change if
> + * there are OUTPUT buffers allocated.
> + */
> + if (vb2_is_busy(vq))
> + return -EBUSY;
> }
>
> ret = vidioc_try_fmt_out_mplane(file, priv, f);
I think this needs to be moved up.
regards
Philipp
Re: [RFC 08/12] media: hantro: Fix H264 decoding of field encoded content
On Tue, 2019-09-03 at 14:02 +, Jonas Karlman wrote: > On 2019-09-03 15:21, Philipp Zabel wrote: > > On Sun, 2019-09-01 at 12:45 +, Jonas Karlman wrote: > > > This need code cleanup and formatting > > > > > > Signed-off-by: Jonas Karlman > > > > The previous patches all work, but this patch breaks decoding of > > progressive content for me (i.MX8MQ with FFmpeg based on Ezequiel's > > branch). > > Please try with ffmpeg based on my v4l2-request-hwaccel-4.0.4-hantro branch > at [1], > up to and including the commit "HACK: add dpb flags for reference usage and > field picture". > That commit adds code to set reference flags needed by the changes in this > patch. > > There is probably also some other minor difference between our two ffmpeg > branches. > I have not observed any issues with progressive content with this patch and > my ffmpeg branch (on a RK3288 device). > Some H264 reference samples do have visual issues after this patch, however > all my real world samples does seem to work. > > My branch use libudev to probe media/video devices and needs to be configured > with: > --enable-v4l2-request --enable-libudev --enable-libdrm > > [1] https://github.com/Kwiboo/FFmpeg/commits/v4l2-request-hwaccel-4.0.4-hantro I hadn't realized that this is a backwards incompatible change. With your branch rebased onto n4.2, and this patch applied, decoding seems to work. regards Philipp
Re: [RFC 08/12] media: hantro: Fix H264 decoding of field encoded content
On Sun, 2019-09-01 at 12:45 +, Jonas Karlman wrote:
> This need code cleanup and formatting
>
> Signed-off-by: Jonas Karlman
The previous patches all work, but this patch breaks decoding of
progressive content for me (i.MX8MQ with FFmpeg based on Ezequiel's
branch).
regards
Philipp
> ---
> .../staging/media/hantro/hantro_g1_h264_dec.c | 26 ++--
> drivers/staging/media/hantro/hantro_h264.c| 126 --
> drivers/staging/media/hantro/hantro_hw.h | 4 +
> 3 files changed, 100 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> index 16f21d258f6a..bc628ef73b29 100644
> --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> @@ -130,28 +130,20 @@ static void set_params(struct hantro_ctx *ctx)
>
> static void set_ref(struct hantro_ctx *ctx)
> {
> + const struct v4l2_ctrl_h264_decode_params *dec_param;
> + const struct v4l2_ctrl_h264_slice_params *slice;
> struct v4l2_h264_dpb_entry *dpb = ctx->h264_dec.dpb;
> const u8 *b0_reflist, *b1_reflist, *p_reflist;
> struct hantro_dev *vpu = ctx->dev;
> - u32 dpb_longterm = 0;
> - u32 dpb_valid = 0;
> int reg_num;
> u32 reg;
> int i;
>
> - /*
> - * Set up bit maps of valid and long term DPBs.
> - * NOTE: The bits are reversed, i.e. MSb is DPB 0.
> - */
> - for (i = 0; i < HANTRO_H264_DPB_SIZE; ++i) {
> - if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> - dpb_valid |= BIT(HANTRO_H264_DPB_SIZE - 1 - i);
> + dec_param = ctx->h264_dec.ctrls.decode;
> + slice = ctx->h264_dec.ctrls.slices;
>
> - if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)
> - dpb_longterm |= BIT(HANTRO_H264_DPB_SIZE - 1 - i);
> - }
> - vdpu_write_relaxed(vpu, dpb_valid << 16, G1_REG_VALID_REF);
> - vdpu_write_relaxed(vpu, dpb_longterm << 16, G1_REG_LT_REF);
> + vdpu_write_relaxed(vpu, ctx->h264_dec.dpb_valid, G1_REG_VALID_REF);
> + vdpu_write_relaxed(vpu, ctx->h264_dec.dpb_longterm, G1_REG_LT_REF);
>
> /*
>* Set up reference frame picture numbers.
> @@ -223,10 +215,8 @@ static void set_ref(struct hantro_ctx *ctx)
>
> /* Set up addresses of DPB buffers. */
> for (i = 0; i < HANTRO_H264_DPB_SIZE; i++) {
> - struct vb2_buffer *buf = hantro_h264_get_ref_buf(ctx, i);
> -
> - vdpu_write_relaxed(vpu, vb2_dma_contig_plane_dma_addr(buf, 0),
> -G1_REG_ADDR_REF(i));
> + dma_addr_t addr = hantro_h264_get_ref_dma_addr(ctx, i);
> + vdpu_write_relaxed(vpu, addr, G1_REG_ADDR_REF(i));
> }
> }
>
> diff --git a/drivers/staging/media/hantro/hantro_h264.c
> b/drivers/staging/media/hantro/hantro_h264.c
> index a77cc28e180a..85c86d728b1a 100644
> --- a/drivers/staging/media/hantro/hantro_h264.c
> +++ b/drivers/staging/media/hantro/hantro_h264.c
> @@ -228,17 +228,65 @@ static void prepare_table(struct hantro_ctx *ctx)
> {
> const struct hantro_h264_dec_ctrls *ctrls = >h264_dec.ctrls;
> const struct v4l2_ctrl_h264_decode_params *dec_param = ctrls->decode;
> + const struct v4l2_ctrl_h264_slice_params *slices = ctrls->slices;
> struct hantro_h264_dec_priv_tbl *tbl = ctx->h264_dec.priv.cpu;
> const struct v4l2_h264_dpb_entry *dpb = ctx->h264_dec.dpb;
> + u32 dpb_longterm = 0;
> + u32 dpb_valid = 0;
> int i;
>
> + /*
> + * Set up bit maps of valid and long term DPBs.
> + * NOTE: The bits are reversed, i.e. MSb is DPB 0.
> + */
> + if ((slices[0].flags & V4L2_H264_SLICE_FLAG_FIELD_PIC) ||
> (slices[0].flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)) {
> + for (i = 0; i < HANTRO_H264_DPB_SIZE * 2; ++i) {
> + // check for correct reference use
> + u32 flag = (i & 0x1) ?
> V4L2_H264_DPB_ENTRY_FLAG_REF_BOTTOM : V4L2_H264_DPB_ENTRY_FLAG_REF_TOP;
> + if (dpb[i / 2].flags & flag)
> + dpb_valid |= BIT(HANTRO_H264_DPB_SIZE * 2 - 1 -
> i);
> +
> + if (dpb[i / 2].flags &
> V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)
> + dpb_longterm |= BIT(HANTRO_H264_DPB_SIZE * 2 -
> 1 - i);
> + }
> +
> + ctx->h264_dec.dpb_valid = dpb_valid;
> + ctx->h264_dec.dpb_longterm = dpb_longterm;
> + } else {
> + for (i = 0; i < HANTRO_H264_DPB_SIZE; ++i) {
> + if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> + dpb_valid |= BIT(HANTRO_H264_DPB_SIZE - 1 - i);
> +
> + if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)
> + dpb_longterm |= BIT(HANTRO_H264_DPB_SIZE - 1 -
> i);
> + }
> +
> +
