* Re: [PATCH v8 11/21] clk: tegra: clk-dfll: Add suspend and resume support
From: Sowjanya Komatineni @ 2019-08-09 18:33 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree, rjw, viresh.kumar, linux-pm
In-Reply-To: <84a0d46a-bca2-1000-a2a6-8890ee702dd3@gmail.com>
On 8/9/19 11:00 AM, Dmitry Osipenko wrote:
> 09.08.2019 19:39, Sowjanya Komatineni пишет:
>> On 8/9/19 5:23 AM, Dmitry Osipenko wrote:
>>> 09.08.2019 2:46, Sowjanya Komatineni пишет:
>>>> This patch implements DFLL suspend and resume operation.
>>>>
>>>> During system suspend entry, CPU clock will switch CPU to safe
>>>> clock source of PLLP and disables DFLL clock output.
>>>>
>>>> DFLL driver suspend confirms DFLL disable state and errors out on
>>>> being active.
>>>>
>>>> DFLL is re-initialized during the DFLL driver resume as it goes
>>>> through complete reset during suspend entry.
>>>>
>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>> ---
>>>> drivers/clk/tegra/clk-dfll.c | 56 ++++++++++++++++++++++++++++++
>>>> drivers/clk/tegra/clk-dfll.h | 2 ++
>>>> drivers/clk/tegra/clk-tegra124-dfll-fcpu.c | 1 +
>>>> 3 files changed, 59 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
>>>> index f8688c2ddf1a..eb298a5d7be9 100644
>>>> --- a/drivers/clk/tegra/clk-dfll.c
>>>> +++ b/drivers/clk/tegra/clk-dfll.c
>>>> @@ -1487,6 +1487,7 @@ static int dfll_init(struct tegra_dfll *td)
>>>> td->last_unrounded_rate = 0;
>>>> pm_runtime_enable(td->dev);
>>>> + pm_runtime_irq_safe(td->dev);
>>>> pm_runtime_get_sync(td->dev);
>>>> dfll_set_mode(td, DFLL_DISABLED);
>>>> @@ -1513,6 +1514,61 @@ static int dfll_init(struct tegra_dfll *td)
>>>> return ret;
>>>> }
>>>> +/**
>>>> + * tegra_dfll_suspend - check DFLL is disabled
>>>> + * @dev: DFLL device *
>>>> + *
>>>> + * DFLL clock should be disabled by the CPUFreq driver. So, make
>>>> + * sure it is disabled and disable all clocks needed by the DFLL.
>>>> + */
>>>> +int tegra_dfll_suspend(struct device *dev)
>>>> +{
>>>> + struct tegra_dfll *td = dev_get_drvdata(dev);
>>>> +
>>>> + if (dfll_is_running(td)) {
>>>> + dev_err(td->dev, "dfll is enabled while shouldn't be\n");
>>>> + return -EBUSY;
>>>> + }
>>>> +
>>>> + reset_control_assert(td->dvco_rst);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +EXPORT_SYMBOL(tegra_dfll_suspend);
>>>> +
>>>> +/**
>>>> + * tegra_dfll_resume - reinitialize DFLL on resume
>>>> + * @dev: DFLL instance
>>>> + *
>>>> + * DFLL is disabled and reset during suspend and resume.
>>>> + * So, reinitialize the DFLL IP block back for use.
>>>> + * DFLL clock is enabled later in closed loop mode by CPUFreq
>>>> + * driver before switching its clock source to DFLL output.
>>>> + */
>>>> +int tegra_dfll_resume(struct device *dev)
>>>> +{
>>>> + struct tegra_dfll *td = dev_get_drvdata(dev);
>>>> +
>>>> + reset_control_deassert(td->dvco_rst);
>>> This doesn't look right because I assume that DFLL resetting is
>>> synchronous and thus clk should be enabled in order for reset to
>>> propagate inside hardware.
>>>
>>>> + pm_runtime_get_sync(td->dev);
>>> Hence it will be better to remove the above reset_control_deassert() and
>>> add here:
>>>
>>> reset_control_reset(td->dvco_rst);
>> By the time dfll resume happens, dfll controller clock will already be enabled.
>>
>> so doing reset de-assert before pm_runtime seems ok.
> I don't see what enables the DFLL clock because it should be enabled by the CPUFreq driver
> on resume from suspend and resume happens after resuming of the DFLL driver.
dvco_rst is part of peripheral clocks and all peripheral clocks are
restored by clk-tegra210 driver which happens before dfll driver resume.
So dfll rst thru part of peripheral clock enable is set prior to dfll
reset deassertion
^ permalink raw reply
* Re: [PATCH v8 19/21] soc/tegra: pmc: Configure deep sleep control settings
From: Dmitry Osipenko @ 2019-08-09 18:22 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree, rjw, viresh.kumar, linux-pm
In-Reply-To: <60ea494a-9960-b7fc-3afb-8c17feec9fda@nvidia.com>
09.08.2019 20:24, Sowjanya Komatineni пишет:
>
> On 8/9/19 9:23 AM, Sowjanya Komatineni wrote:
>>
>> On 8/9/19 6:23 AM, Dmitry Osipenko wrote:
>>> 09.08.2019 2:46, Sowjanya Komatineni пишет:
>>>> Tegra210 and prior Tegra chips have deep sleep entry and wakeup related
>>>> timings which are platform specific that should be configured before
>>>> entering into deep sleep.
>>>>
>>>> Below are the timing specific configurations for deep sleep entry and
>>>> wakeup.
>>>> - Core rail power-on stabilization timer
>>>> - OSC clock stabilization timer after SOC rail power is stabilized.
>>>> - Core power off time is the minimum wake delay to keep the system
>>>> in deep sleep state irrespective of any quick wake event.
>>>>
>>>> These values depends on the discharge time of regulators and turn OFF
>>>> time of the PMIC to allow the complete system to finish entering into
>>>> deep sleep state.
>>>>
>>>> These values vary based on the platform design and are specified
>>>> through the device tree.
>>>>
>>>> This patch has implementation to configure these timings which are must
>>>> to have for proper deep sleep and wakeup operations.
>>>>
>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>> ---
>>>> drivers/soc/tegra/pmc.c | 13 ++++++++++++-
>>>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>>>> index e013ada7e4e9..9a78d8417367 100644
>>>> --- a/drivers/soc/tegra/pmc.c
>>>> +++ b/drivers/soc/tegra/pmc.c
>>>> @@ -88,6 +88,8 @@
>>>> #define PMC_CPUPWRGOOD_TIMER 0xc8
>>>> #define PMC_CPUPWROFF_TIMER 0xcc
>>>> +#define PMC_COREPWRGOOD_TIMER 0x3c
>>>> +#define PMC_COREPWROFF_TIMER 0xe0
>>>> #define PMC_PWR_DET_VALUE 0xe4
>>>> @@ -2277,7 +2279,7 @@ static const struct tegra_pmc_regs tegra20_pmc_regs = {
>>>> static void tegra20_pmc_init(struct tegra_pmc *pmc)
>>>> {
>>>> - u32 value;
>>>> + u32 value, osc, pmu, off;
>>>> /* Always enable CPU power request */
>>>> value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>>> @@ -2303,6 +2305,15 @@ static void tegra20_pmc_init(struct tegra_pmc *pmc)
>>>> value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>>> value |= PMC_CNTRL_SYSCLK_OE;
>>>> tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>> +
>>>> + osc = DIV_ROUND_UP(pmc->core_osc_time * 8192, 1000000);
>>>> + pmu = DIV_ROUND_UP(pmc->core_pmu_time * 32768, 1000000);
>>>> + off = DIV_ROUND_UP(pmc->core_off_time * 32768, 1000000);
>>>> + if (osc && pmu)
>>>> + tegra_pmc_writel(pmc, ((osc << 8) & 0xff00) | (pmu & 0xff),
>>>> + PMC_COREPWRGOOD_TIMER);
>>>> + if (off)
>>>> + tegra_pmc_writel(pmc, off, PMC_COREPWROFF_TIMER);
>>> The osc/pmu/off values are undefined if they are not defined in device-tree. I suppose this
>>> need to be corrected in tegra_pmc_parse_dt() if the values really matter even if LP0 suspend
>>> isn't supported in device-tree.
>>>
>>> And I'm also not sure what's wrong with setting 0 for the timers.
>>>
>> These settings are for SC7 only and will not have any impact in normal state.
> POR value for these timing registers is not 0 and has default timings based on chip design
> and on top of that based on platform HW components charge/discharge timings there's a need
> to increase these timings so support for programming these thru DT is needed and these
> values have effect only in LP0.
If it is legal to omit the timings in DT, then tegra_pmc_parse_dt() need to be corrected if
there is assumption of 0 meaning that values are undefined in DT.
^ permalink raw reply
* Re: [PATCH v3 04/14] net: phy: adin: add {write,read}_mmd hooks
From: Heiner Kallweit @ 2019-08-09 18:21 UTC (permalink / raw)
To: Alexandru Ardelean, netdev, devicetree, linux-kernel
Cc: davem, robh+dt, mark.rutland, f.fainelli, andrew
In-Reply-To: <20190809133552.21597-5-alexandru.ardelean@analog.com>
On 09.08.2019 15:35, Alexandru Ardelean wrote:
> Both ADIN1200 & ADIN1300 support Clause 45 access for some registers.
> The Extended Management Interface (EMI) registers are accessible via both
> Clause 45 (at register MDIO_MMD_VEND1) and using Clause 22.
>
> However, the Clause 22 access for MMD regs differs from the standard one
> defined by 802.3. The ADIN PHYs use registers ExtRegPtr (0x0010) and
> ExtRegData (0x0011) to access Clause 45 & EMI registers.
>
> The indirect access is done via the following mechanism (for both R/W):
> 1. Write the address of the register in the ExtRegPtr
> 2. Read/write the value of the register (written at ExtRegPtr)
>
> This mechanism is needed to manage configuration of chip settings and to
> access EEE registers (via Clause 22).
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> drivers/net/phy/adin.c | 46 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 46 insertions(+)
>
> diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> index 91ff26d08fd5..8973ad819b93 100644
> --- a/drivers/net/phy/adin.c
> +++ b/drivers/net/phy/adin.c
> @@ -14,6 +14,9 @@
> #define PHY_ID_ADIN1200 0x0283bc20
> #define PHY_ID_ADIN1300 0x0283bc30
>
> +#define ADIN1300_MII_EXT_REG_PTR 0x0010
> +#define ADIN1300_MII_EXT_REG_DATA 0x0011
> +
> #define ADIN1300_INT_MASK_REG 0x0018
> #define ADIN1300_INT_MDIO_SYNC_EN BIT(9)
> #define ADIN1300_INT_ANEG_STAT_CHNG_EN BIT(8)
> @@ -53,6 +56,45 @@ static int adin_phy_config_intr(struct phy_device *phydev)
> ADIN1300_INT_MASK_EN);
> }
>
> +static int adin_read_mmd(struct phy_device *phydev, int devad, u16 regnum)
> +{
> + struct mii_bus *bus = phydev->mdio.bus;
> + int phy_addr = phydev->mdio.addr;
> + int err;
> +
> + if (phydev->is_c45) {
Similar to what we discussed regarding feature detection:
Flag is_c45 shouldn't be set with these PHY's, therefore this is dead code.
> + u32 addr = MII_ADDR_C45 | (devad << 16) | (regnum & 0xffff);
> +
> + return __mdiobus_read(bus, phy_addr, addr);
> + }
> +
> + err = __mdiobus_write(bus, phy_addr, ADIN1300_MII_EXT_REG_PTR, regnum);
> + if (err)
> + return err;
> +
> + return __mdiobus_read(bus, phy_addr, ADIN1300_MII_EXT_REG_DATA);
> +}
> +
> +static int adin_write_mmd(struct phy_device *phydev, int devad, u16 regnum,
> + u16 val)
> +{
> + struct mii_bus *bus = phydev->mdio.bus;
> + int phy_addr = phydev->mdio.addr;
> + int err;
> +
> + if (phydev->is_c45) {
> + u32 addr = MII_ADDR_C45 | (devad << 16) | (regnum & 0xffff);
> +
> + return __mdiobus_write(bus, phy_addr, addr, val);
> + }
> +
> + err = __mdiobus_write(bus, phy_addr, ADIN1300_MII_EXT_REG_PTR, regnum);
> + if (err)
> + return err;
> +
> + return __mdiobus_write(bus, phy_addr, ADIN1300_MII_EXT_REG_DATA, val);
> +}
> +
> static struct phy_driver adin_driver[] = {
> {
> PHY_ID_MATCH_MODEL(PHY_ID_ADIN1200),
> @@ -64,6 +106,8 @@ static struct phy_driver adin_driver[] = {
> .config_intr = adin_phy_config_intr,
> .resume = genphy_resume,
> .suspend = genphy_suspend,
> + .read_mmd = adin_read_mmd,
> + .write_mmd = adin_write_mmd,
> },
> {
> PHY_ID_MATCH_MODEL(PHY_ID_ADIN1300),
> @@ -75,6 +119,8 @@ static struct phy_driver adin_driver[] = {
> .config_intr = adin_phy_config_intr,
> .resume = genphy_resume,
> .suspend = genphy_suspend,
> + .read_mmd = adin_read_mmd,
> + .write_mmd = adin_write_mmd,
> },
> };
>
>
^ permalink raw reply
* Re: [PATCH v3 03/14] net: phy: adin: add support for interrupts
From: Heiner Kallweit @ 2019-08-09 18:19 UTC (permalink / raw)
To: Alexandru Ardelean, netdev, devicetree, linux-kernel
Cc: davem, robh+dt, mark.rutland, f.fainelli, andrew
In-Reply-To: <20190809133552.21597-4-alexandru.ardelean@analog.com>
On 09.08.2019 15:35, Alexandru Ardelean wrote:
> This change adds support for enabling PHY interrupts that can be used by
> the PHY framework to get signal for link/speed/auto-negotiation changes.
>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> drivers/net/phy/adin.c | 38 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
> diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> index fc0148ba4b94..91ff26d08fd5 100644
> --- a/drivers/net/phy/adin.c
> +++ b/drivers/net/phy/adin.c
> @@ -14,11 +14,45 @@
> #define PHY_ID_ADIN1200 0x0283bc20
> #define PHY_ID_ADIN1300 0x0283bc30
>
> +#define ADIN1300_INT_MASK_REG 0x0018
> +#define ADIN1300_INT_MDIO_SYNC_EN BIT(9)
> +#define ADIN1300_INT_ANEG_STAT_CHNG_EN BIT(8)
> +#define ADIN1300_INT_ANEG_PAGE_RX_EN BIT(6)
> +#define ADIN1300_INT_IDLE_ERR_CNT_EN BIT(5)
> +#define ADIN1300_INT_MAC_FIFO_OU_EN BIT(4)
> +#define ADIN1300_INT_RX_STAT_CHNG_EN BIT(3)
> +#define ADIN1300_INT_LINK_STAT_CHNG_EN BIT(2)
> +#define ADIN1300_INT_SPEED_CHNG_EN BIT(1)
> +#define ADIN1300_INT_HW_IRQ_EN BIT(0)
> +#define ADIN1300_INT_MASK_EN \
> + (ADIN1300_INT_ANEG_STAT_CHNG_EN | ADIN1300_INT_ANEG_PAGE_RX_EN | \
> + ADIN1300_INT_LINK_STAT_CHNG_EN | ADIN1300_INT_SPEED_CHNG_EN | \
> + ADIN1300_INT_HW_IRQ_EN)
phylib only needs the link status change interrupt. It shouldn't hurt
if enable more interrupt sources, but it's not needed.
> +#define ADIN1300_INT_STATUS_REG 0x0019
> +
> static int adin_config_init(struct phy_device *phydev)
> {
> return genphy_config_init(phydev);
> }
>
> +static int adin_phy_ack_intr(struct phy_device *phydev)
> +{
> + /* Clear pending interrupts */
> + int rc = phy_read(phydev, ADIN1300_INT_STATUS_REG);
> +
> + return rc < 0 ? rc : 0;
> +}
> +
> +static int adin_phy_config_intr(struct phy_device *phydev)
> +{
> + if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
> + return phy_set_bits(phydev, ADIN1300_INT_MASK_REG,
> + ADIN1300_INT_MASK_EN);
> +
> + return phy_clear_bits(phydev, ADIN1300_INT_MASK_REG,
> + ADIN1300_INT_MASK_EN);
> +}
> +
> static struct phy_driver adin_driver[] = {
> {
> PHY_ID_MATCH_MODEL(PHY_ID_ADIN1200),
> @@ -26,6 +60,8 @@ static struct phy_driver adin_driver[] = {
> .config_init = adin_config_init,
> .config_aneg = genphy_config_aneg,
> .read_status = genphy_read_status,
> + .ack_interrupt = adin_phy_ack_intr,
> + .config_intr = adin_phy_config_intr,
> .resume = genphy_resume,
> .suspend = genphy_suspend,
> },
> @@ -35,6 +71,8 @@ static struct phy_driver adin_driver[] = {
> .config_init = adin_config_init,
> .config_aneg = genphy_config_aneg,
> .read_status = genphy_read_status,
> + .ack_interrupt = adin_phy_ack_intr,
> + .config_intr = adin_phy_config_intr,
> .resume = genphy_resume,
> .suspend = genphy_suspend,
> },
>
^ permalink raw reply
* Re: [PATCH v8 14/21] clk: tegra210: Add suspend and resume support
From: Dmitry Osipenko @ 2019-08-09 18:18 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree, rjw, viresh.kumar, linux-pm
In-Reply-To: <7d101ec9-c559-8b40-1764-6bf67a9c7a7a@nvidia.com>
09.08.2019 19:19, Sowjanya Komatineni пишет:
>
> On 8/9/19 6:56 AM, Dmitry Osipenko wrote:
>> 09.08.2019 2:46, Sowjanya Komatineni пишет:
>>> This patch adds support for clk: tegra210: suspend-resume.
>>>
>>> All the CAR controller settings are lost on suspend when core
>>> power goes off.
>>>
>>> This patch has implementation for saving and restoring all PLLs
>>> and clocks context during system suspend and resume to have the
>>> clocks back to same state for normal operation.
>>>
>>> Clock driver suspend and resume are registered as syscore_ops as clocks
>>> restore need to happen before the other drivers resume to have all their
>>> clocks back to the same state as before suspend.
>>>
>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>> ---
>>> drivers/clk/tegra/clk-tegra210.c | 103 +++++++++++++++++++++++++++++++++++++--
>>> drivers/clk/tegra/clk.c | 64 ++++++++++++++++++++++++
>>> drivers/clk/tegra/clk.h | 3 ++
>>> 3 files changed, 166 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/clk/tegra/clk-tegra210.c b/drivers/clk/tegra/clk-tegra210.c
>>> index 998bf60b219a..8dd6f4f4debb 100644
>>> --- a/drivers/clk/tegra/clk-tegra210.c
>>> +++ b/drivers/clk/tegra/clk-tegra210.c
>>> @@ -9,13 +9,13 @@
>>> #include <linux/clkdev.h>
>>> #include <linux/of.h>
>>> #include <linux/of_address.h>
>>> +#include <linux/syscore_ops.h>
>>> #include <linux/delay.h>
>>> #include <linux/export.h>
>>> #include <linux/mutex.h>
>>> #include <linux/clk/tegra.h>
>>> #include <dt-bindings/clock/tegra210-car.h>
>>> #include <dt-bindings/reset/tegra210-car.h>
>>> -#include <linux/iopoll.h>
>>> #include <linux/sizes.h>
>>> #include <soc/tegra/pmc.h>
>>> @@ -220,11 +220,15 @@
>>> #define CLK_M_DIVISOR_SHIFT 2
>>> #define CLK_M_DIVISOR_MASK 0x3
>>> +#define CLK_MASK_ARM 0x44
>>> +#define MISC_CLK_ENB 0x48
>>> +
>>> #define RST_DFLL_DVCO 0x2f4
>>> #define DVFS_DFLL_RESET_SHIFT 0
>>> #define CLK_RST_CONTROLLER_RST_DEV_Y_SET 0x2a8
>>> #define CLK_RST_CONTROLLER_RST_DEV_Y_CLR 0x2ac
>>> +#define CPU_SOFTRST_CTRL 0x380
>>> #define LVL2_CLK_GATE_OVRA 0xf8
>>> #define LVL2_CLK_GATE_OVRC 0x3a0
>>> @@ -2825,6 +2829,7 @@ static int tegra210_enable_pllu(void)
>>> struct tegra_clk_pll_freq_table *fentry;
>>> struct tegra_clk_pll pllu;
>>> u32 reg;
>>> + int ret;
>>> for (fentry = pll_u_freq_table; fentry->input_rate; fentry++) {
>>> if (fentry->input_rate == pll_ref_freq)
>>> @@ -2853,9 +2858,14 @@ static int tegra210_enable_pllu(void)
>>> reg |= PLL_ENABLE;
>>> writel(reg, clk_base + PLLU_BASE);
>>> - readl_relaxed_poll_timeout_atomic(clk_base + PLLU_BASE, reg,
>>> - reg & PLL_BASE_LOCK, 2, 1000);
>>> - if (!(reg & PLL_BASE_LOCK)) {
>>> + /*
>>> + * During clocks resume, same PLLU init and enable sequence get
>>> + * executed. So, readx_poll_timeout_atomic can't be used here as it
>>> + * uses ktime_get() and timekeeping resume doesn't happen by that
>>> + * time. So, using tegra210_wait_for_mask for PLL LOCK.
>>> + */
>>> + ret = tegra210_wait_for_mask(&pllu, PLLU_BASE, PLL_BASE_LOCK);
>>> + if (ret) {
>>> pr_err("Timed out waiting for PLL_U to lock\n");
>>> return -ETIMEDOUT;
>>> }
>>> @@ -3288,6 +3298,84 @@ static void tegra210_disable_cpu_clock(u32 cpu)
>>> }
>>> #ifdef CONFIG_PM_SLEEP
>>> +/*
>>> + * This array lists mask values for each peripheral clk bank
>>> + * to mask out reserved bits during the clocks state restore
>>> + * on SC7 resume to prevent accidental writes to these reserved
>>> + * bits.
>>> + */
>>> +static u32 periph_clk_rsvd_mask[TEGRA210_CAR_BANK_COUNT] = {
>> Should be more natural to have a "valid_mask" instead of "rsvd_mask".
>>
>> What's actually wrong with touching of the reserved bits? They must be NO-OP.. or the
>> reserved bits are actually some kind of "secret" bits? If those bits have some use-case
>> outside of Silicon HW (like FPGA simulation), then this doesn't matter for upstream and you
>> have to keep the workaround locally in the downstream kernel or whatever.
>
> Will rename as valid_mask.
>
> some bits in these registers are undefined and is not good to write to these bits as they
> can cause pslverr.
Okay, it should be explained in the comment.
Is it possible to disable trapping of changing the undefined bits?
>>
>>> + 0x23282006,
>>> + 0x782e0c18,
>>> + 0x0c012c05,
>>> + 0x003e7304,
>>> + 0x86c04800,
>>> + 0xc0199000,
>>> + 0x03e03800,
>>> +};
>>> +
>>> +#define car_readl(_base, _off) readl_relaxed(clk_base + (_base) + ((_off) * 4))
>>> +#define car_writel(_val, _base, _off) \
>>> + writel_relaxed(_val, clk_base + (_base) + ((_off) * 4))
>>> +
>>> +static u32 spare_reg_ctx, misc_clk_enb_ctx, clk_msk_arm_ctx;
>>> +static u32 cpu_softrst_ctx[3];
>>> +
>>> +static int tegra210_clk_suspend(void)
>>> +{
>>> + unsigned int i;
>>> +
>>> + clk_save_context();
>>> +
>>> + /*
>>> + * Save the bootloader configured clock registers SPARE_REG0,
>>> + * MISC_CLK_ENB, CLK_MASK_ARM, CPU_SOFTRST_CTRL.
>>> + */
>>> + spare_reg_ctx = readl_relaxed(clk_base + SPARE_REG0);
>>> + misc_clk_enb_ctx = readl_relaxed(clk_base + MISC_CLK_ENB);
>>> + clk_msk_arm_ctx = readl_relaxed(clk_base + CLK_MASK_ARM);
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
>>> + cpu_softrst_ctx[i] = car_readl(CPU_SOFTRST_CTRL, i);
>>> +
>>> + tegra_clk_periph_suspend();
>>> + return 0;
>>> +}
>>> +
>>> +static void tegra210_clk_resume(void)
>>> +{
>>> + unsigned int i;
>>> +
>>> + tegra_clk_osc_resume(clk_base);
>>> +
>>> + /*
>>> + * Restore the bootloader configured clock registers SPARE_REG0,
>>> + * MISC_CLK_ENB, CLK_MASK_ARM, CPU_SOFTRST_CTRL from saved context.
>>> + */
>>> + writel_relaxed(spare_reg_ctx, clk_base + SPARE_REG0);
>>> + writel_relaxed(misc_clk_enb_ctx, clk_base + MISC_CLK_ENB);
>>> + writel_relaxed(clk_msk_arm_ctx, clk_base + CLK_MASK_ARM);
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
>>> + car_writel(cpu_softrst_ctx[i], CPU_SOFTRST_CTRL, i);
>>> +
>>> + fence_udelay(5, clk_base);
>>> +
>>> + /* enable all the clocks before changing the clock sources */
>>> + tegra_clk_periph_force_on(periph_clk_rsvd_mask);
>> Why clocks need to be enabled before changing the sources?
>
> To prevent glitchless frequency switch, Tegra clock programming recommended sequence is to
> change MUX control or divisor or both with the clocks running.
This should be explained in the comment.
> Actual state of clocks before suspend are restored later after all PLL's and peripheral
> clocks are restored.
>
>>
>>> + /* wait for all writes to happen to have all the clocks enabled */
>>> + wmb();
>> fence_udelay() has exactly the same barrier at the very beginning of readl(), no need to
>> duplicate it here.
Actually, readl does the rmb() and it should be a more correct variant of fencing because it
actually ensures that the write reached hardware. I suppose that something like fence_udelay
should be used for the pinctrl as well.
>>> + fence_udelay(2, clk_base);
>>> +
>>> + /* restore PLLs and all peripheral clock rates */
>>> + tegra210_init_pllu();
>> Why USB PLL need to be restored at first?
> USB PLL restore is independent to all other clocks restore. So this can be done either
> before clk_restore_context or even after.
Then why not to implement restore_context for PLLU?
>>> + clk_restore_context();
>>> +
>>> + /* restore all peripheral clocks enable and reset state */
>>> + tegra_clk_periph_resume();
>>> +}
>> [snip]
^ permalink raw reply
* Re: [PATCH v2 2/2] spi: npcm-fiu: add NPCM FIU controller driver
From: Benjamin Fair @ 2019-08-09 18:01 UTC (permalink / raw)
To: Tomer Maimon
Cc: broonie, robh+dt, mark.rutland, vigneshr, bbrezillon,
avifishman70, Tali Perry, Patrick Venture, Nancy Yuen, linux-spi,
devicetree, OpenBMC Maillist, linux-kernel
In-Reply-To: <20190808131448.349161-3-tmaimon77@gmail.com>
On Thu, Aug 8, 2019 at 6:15 AM Tomer Maimon <tmaimon77@gmail.com> wrote:
>
> Add Nuvoton NPCM BMC Flash Interface Unit(FIU) SPI master
> controller driver using SPI-MEM interface.
>
> The FIU supports single, dual or quad communication interface.
>
> the FIU controller can operate in following modes:
> - User Mode Access(UMA): provides flash access by using an
> indirect address/data mechanism.
> - direct rd/wr mode: maps the flash memory into the core
> address space.
> - SPI-X mode: used for an expansion bus to an ASIC or CPLD.
>
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> ---
> drivers/spi/Kconfig | 10 +
> drivers/spi/Makefile | 1 +
> drivers/spi/spi-npcm-fiu.c | 761 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 772 insertions(+)
> create mode 100644 drivers/spi/spi-npcm-fiu.c
>
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 3a1d8f1170de..6ee514fd0920 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -433,6 +433,16 @@ config SPI_MT7621
> help
> This selects a driver for the MediaTek MT7621 SPI Controller.
>
> +config SPI_NPCM_FIU
> + tristate "Nuvoton NPCM FLASH Interface Unit"
> + depends on ARCH_NPCM || COMPILE_TEST
> + depends on OF && HAS_IOMEM
> + help
> + This enables support for the Flash Interface Unit SPI controller
> + in master mode.
> + This driver does not support generic SPI. The implementation only
> + supports spi-mem interface.
> +
> config SPI_NPCM_PSPI
> tristate "Nuvoton NPCM PSPI Controller"
> depends on ARCH_NPCM || COMPILE_TEST
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 63dcab552bcb..adbebee93a75 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -63,6 +63,7 @@ obj-$(CONFIG_SPI_MT65XX) += spi-mt65xx.o
> obj-$(CONFIG_SPI_MT7621) += spi-mt7621.o
> obj-$(CONFIG_SPI_MXIC) += spi-mxic.o
> obj-$(CONFIG_SPI_MXS) += spi-mxs.o
> +obj-$(CONFIG_SPI_NPCM_FIU) += spi-npcm-fiu.o
> obj-$(CONFIG_SPI_NPCM_PSPI) += spi-npcm-pspi.o
> obj-$(CONFIG_SPI_NUC900) += spi-nuc900.o
> obj-$(CONFIG_SPI_NXP_FLEXSPI) += spi-nxp-fspi.o
> diff --git a/drivers/spi/spi-npcm-fiu.c b/drivers/spi/spi-npcm-fiu.c
> new file mode 100644
> index 000000000000..2d8c281e8fa9
> --- /dev/null
> +++ b/drivers/spi/spi-npcm-fiu.c
> @@ -0,0 +1,761 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2019 Nuvoton Technology corporation.
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/ioport.h>
> +#include <linux/clk.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/vmalloc.h>
> +#include <linux/regmap.h>
> +#include <linux/of_device.h>
> +#include <linux/spi/spi-mem.h>
> +#include <linux/mfd/syscon.h>
> +
> +/* NPCM7xx GCR module */
> +#define NPCM7XX_INTCR3_OFFSET 0x9C
> +#define NPCM7XX_INTCR3_FIU_FIX BIT(6)
> +
> +/* Flash Interface Unit (FIU) Registers */
> +#define NPCM_FIU_DRD_CFG 0x00
> +#define NPCM_FIU_DWR_CFG 0x04
> +#define NPCM_FIU_UMA_CFG 0x08
> +#define NPCM_FIU_UMA_CTS 0x0C
> +#define NPCM_FIU_UMA_CMD 0x10
> +#define NPCM_FIU_UMA_ADDR 0x14
> +#define NPCM_FIU_PRT_CFG 0x18
> +#define NPCM_FIU_UMA_DW0 0x20
> +#define NPCM_FIU_UMA_DW1 0x24
> +#define NPCM_FIU_UMA_DW2 0x28
> +#define NPCM_FIU_UMA_DW3 0x2C
> +#define NPCM_FIU_UMA_DR0 0x30
> +#define NPCM_FIU_UMA_DR1 0x34
> +#define NPCM_FIU_UMA_DR2 0x38
> +#define NPCM_FIU_UMA_DR3 0x3C
> +#define NPCM_FIU_MAX_REG_LIMIT 0x80
> +
> +/* FIU Direct Read Configuration Register */
> +#define NPCM_FIU_DRD_CFG_LCK BIT(31)
> +#define NPCM_FIU_DRD_CFG_R_BURST GENMASK(25, 24)
> +#define NPCM_FIU_DRD_CFG_ADDSIZ GENMASK(17, 16)
> +#define NPCM_FIU_DRD_CFG_DBW GENMASK(13, 12)
> +#define NPCM_FIU_DRD_CFG_ACCTYPE GENMASK(9, 8)
> +#define NPCM_FIU_DRD_CFG_RDCMD GENMASK(7, 0)
> +#define NPCM_FIU_DRD_ADDSIZ_SHIFT 16
> +#define NPCM_FIU_DRD_DBW_SHIFT 12
> +#define NPCM_FIU_DRD_ACCTYPE_SHIFT 8
> +
> +/* FIU Direct Write Configuration Register */
> +#define NPCM_FIU_DWR_CFG_LCK BIT(31)
> +#define NPCM_FIU_DWR_CFG_W_BURST GENMASK(25, 24)
> +#define NPCM_FIU_DWR_CFG_ADDSIZ GENMASK(17, 16)
> +#define NPCM_FIU_DWR_CFG_ABPCK GENMASK(11, 10)
> +#define NPCM_FIU_DWR_CFG_DBPCK GENMASK(9, 8)
> +#define NPCM_FIU_DWR_CFG_WRCMD GENMASK(7, 0)
> +#define NPCM_FIU_DWR_ADDSIZ_SHIFT 16
> +#define NPCM_FIU_DWR_ABPCK_SHIFT 10
> +#define NPCM_FIU_DWR_DBPCK_SHIFT 8
> +
> +/* FIU UMA Configuration Register */
> +#define NPCM_FIU_UMA_CFG_LCK BIT(31)
> +#define NPCM_FIU_UMA_CFG_CMMLCK BIT(30)
> +#define NPCM_FIU_UMA_CFG_RDATSIZ GENMASK(28, 24)
> +#define NPCM_FIU_UMA_CFG_DBSIZ GENMASK(23, 21)
> +#define NPCM_FIU_UMA_CFG_WDATSIZ GENMASK(20, 16)
> +#define NPCM_FIU_UMA_CFG_ADDSIZ GENMASK(13, 11)
> +#define NPCM_FIU_UMA_CFG_CMDSIZ BIT(10)
> +#define NPCM_FIU_UMA_CFG_RDBPCK GENMASK(9, 8)
> +#define NPCM_FIU_UMA_CFG_DBPCK GENMASK(7, 6)
> +#define NPCM_FIU_UMA_CFG_WDBPCK GENMASK(5, 4)
> +#define NPCM_FIU_UMA_CFG_ADBPCK GENMASK(3, 2)
> +#define NPCM_FIU_UMA_CFG_CMBPCK GENMASK(1, 0)
> +#define NPCM_FIU_UMA_CFG_ADBPCK_SHIFT 2
> +#define NPCM_FIU_UMA_CFG_WDBPCK_SHIFT 4
> +#define NPCM_FIU_UMA_CFG_DBPCK_SHIFT 6
> +#define NPCM_FIU_UMA_CFG_RDBPCK_SHIFT 8
> +#define NPCM_FIU_UMA_CFG_ADDSIZ_SHIFT 11
> +#define NPCM_FIU_UMA_CFG_WDATSIZ_SHIFT 16
> +#define NPCM_FIU_UMA_CFG_DBSIZ_SHIFT 21
> +#define NPCM_FIU_UMA_CFG_RDATSIZ_SHIFT 24
> +
> +/* FIU UMA Control and Status Register */
> +#define NPCM_FIU_UMA_CTS_RDYIE BIT(25)
> +#define NPCM_FIU_UMA_CTS_RDYST BIT(24)
> +#define NPCM_FIU_UMA_CTS_SW_CS BIT(16)
> +#define NPCM_FIU_UMA_CTS_DEV_NUM GENMASK(9, 8)
> +#define NPCM_FIU_UMA_CTS_EXEC_DONE BIT(0)
> +#define NPCM_FIU_UMA_CTS_DEV_NUM_SHIFT 8
> +
> +/* FIU UMA Command Register */
> +#define NPCM_FIU_UMA_CMD_DUM3 GENMASK(31, 24)
> +#define NPCM_FIU_UMA_CMD_DUM2 GENMASK(23, 16)
> +#define NPCM_FIU_UMA_CMD_DUM1 GENMASK(15, 8)
> +#define NPCM_FIU_UMA_CMD_CMD GENMASK(7, 0)
> +
> +/* FIU UMA Address Register */
> +#define NPCM_FIU_UMA_ADDR_UMA_ADDR GENMASK(31, 0)
> +#define NPCM_FIU_UMA_ADDR_AB3 GENMASK(31, 24)
> +#define NPCM_FIU_UMA_ADDR_AB2 GENMASK(23, 16)
> +#define NPCM_FIU_UMA_ADDR_AB1 GENMASK(15, 8)
> +#define NPCM_FIU_UMA_ADDR_AB0 GENMASK(7, 0)
> +
> +/* FIU UMA Write Data Bytes 0-3 Register */
> +#define NPCM_FIU_UMA_DW0_WB3 GENMASK(31, 24)
> +#define NPCM_FIU_UMA_DW0_WB2 GENMASK(23, 16)
> +#define NPCM_FIU_UMA_DW0_WB1 GENMASK(15, 8)
> +#define NPCM_FIU_UMA_DW0_WB0 GENMASK(7, 0)
> +
> +/* FIU UMA Write Data Bytes 4-7 Register */
> +#define NPCM_FIU_UMA_DW1_WB7 GENMASK(31, 24)
> +#define NPCM_FIU_UMA_DW1_WB6 GENMASK(23, 16)
> +#define NPCM_FIU_UMA_DW1_WB5 GENMASK(15, 8)
> +#define NPCM_FIU_UMA_DW1_WB4 GENMASK(7, 0)
> +
> +/* FIU UMA Write Data Bytes 8-11 Register */
> +#define NPCM_FIU_UMA_DW2_WB11 GENMASK(31, 24)
> +#define NPCM_FIU_UMA_DW2_WB10 GENMASK(23, 16)
> +#define NPCM_FIU_UMA_DW2_WB9 GENMASK(15, 8)
> +#define NPCM_FIU_UMA_DW2_WB8 GENMASK(7, 0)
> +
> +/* FIU UMA Write Data Bytes 12-15 Register */
> +#define NPCM_FIU_UMA_DW3_WB15 GENMASK(31, 24)
> +#define NPCM_FIU_UMA_DW3_WB14 GENMASK(23, 16)
> +#define NPCM_FIU_UMA_DW3_WB13 GENMASK(15, 8)
> +#define NPCM_FIU_UMA_DW3_WB12 GENMASK(7, 0)
> +
> +/* FIU UMA Read Data Bytes 0-3 Register */
> +#define NPCM_FIU_UMA_DR0_RB3 GENMASK(31, 24)
> +#define NPCM_FIU_UMA_DR0_RB2 GENMASK(23, 16)
> +#define NPCM_FIU_UMA_DR0_RB1 GENMASK(15, 8)
> +#define NPCM_FIU_UMA_DR0_RB0 GENMASK(7, 0)
> +
> +/* FIU UMA Read Data Bytes 4-7 Register */
> +#define NPCM_FIU_UMA_DR1_RB15 GENMASK(31, 24)
> +#define NPCM_FIU_UMA_DR1_RB14 GENMASK(23, 16)
> +#define NPCM_FIU_UMA_DR1_RB13 GENMASK(15, 8)
> +#define NPCM_FIU_UMA_DR1_RB12 GENMASK(7, 0)
> +
> +/* FIU UMA Read Data Bytes 8-11 Register */
> +#define NPCM_FIU_UMA_DR2_RB15 GENMASK(31, 24)
> +#define NPCM_FIU_UMA_DR2_RB14 GENMASK(23, 16)
> +#define NPCM_FIU_UMA_DR2_RB13 GENMASK(15, 8)
> +#define NPCM_FIU_UMA_DR2_RB12 GENMASK(7, 0)
> +
> +/* FIU UMA Read Data Bytes 12-15 Register */
> +#define NPCM_FIU_UMA_DR3_RB15 GENMASK(31, 24)
> +#define NPCM_FIU_UMA_DR3_RB14 GENMASK(23, 16)
> +#define NPCM_FIU_UMA_DR3_RB13 GENMASK(15, 8)
> +#define NPCM_FIU_UMA_DR3_RB12 GENMASK(7, 0)
> +
> +/* FIU Read Mode */
> +enum {
> + DRD_SINGLE_WIRE_MODE = 0,
> + DRD_DUAL_IO_MODE = 1,
> + DRD_QUAD_IO_MODE = 2,
> + DRD_SPI_X_MODE = 3,
> +};
> +
> +enum {
> + DWR_ABPCK_BIT_PER_CLK = 0,
> + DWR_ABPCK_2_BIT_PER_CLK = 1,
> + DWR_ABPCK_4_BIT_PER_CLK = 2,
> +};
> +
> +enum {
> + DWR_DBPCK_BIT_PER_CLK = 0,
> + DWR_DBPCK_2_BIT_PER_CLK = 1,
> + DWR_DBPCK_4_BIT_PER_CLK = 2,
> +};
> +
> +#define NPCM_FIU_DRD_16_BYTE_BURST 0x3000000
> +#define NPCM_FIU_DWR_16_BYTE_BURST 0x3000000
> +
> +#define MAP_SIZE_128MB 0x8000000
> +#define MAP_SIZE_16MB 0x1000000
> +#define MAP_SIZE_8MB 0x800000
> +
> +#define NUM_BITS_IN_BYTE 8
> +#define FIU_DRD_MAX_DUMMY_NUMBER 3
> +#define NPCM_MAX_CHIP_NUM 4
> +#define CHUNK_SIZE 16
> +#define UMA_MICRO_SEC_TIMEOUT 150
> +
> +enum {
> + FIU0 = 0,
> + FIU3,
> + FIUX,
> +};
> +
> +struct npcm_fiu_info {
> + char *name;
> + u32 fiu_id;
> + u32 max_map_size;
> + u32 max_cs;
> +};
> +
> +struct fiu_data {
> + const struct npcm_fiu_info *npcm_fiu_data_info;
> + int fiu_max;
> +};
> +
> +static const struct npcm_fiu_info npxm7xx_fiu_info[] = {
> + {.name = "FIU0", .fiu_id = FIU0,
> + .max_map_size = MAP_SIZE_128MB, .max_cs = 2},
> + {.name = "FIU3", .fiu_id = FIU3,
> + .max_map_size = MAP_SIZE_128MB, .max_cs = 4},
> + {.name = "FIUX", .fiu_id = FIUX,
> + .max_map_size = MAP_SIZE_16MB, .max_cs = 2} };
> +
> +static const struct fiu_data npxm7xx_fiu_data = {
> + .npcm_fiu_data_info = npxm7xx_fiu_info,
> + .fiu_max = 3,
> +};
> +
> +struct npcm_fiu_spi;
> +
> +struct npcm_fiu_chip {
> + void __iomem *flash_region_mapped_ptr;
> + struct npcm_fiu_spi *fiu;
> + unsigned long clkrate;
> + u32 chipselect;
> +};
> +
> +struct npcm_fiu_spi {
> + struct npcm_fiu_chip chip[NPCM_MAX_CHIP_NUM];
> + const struct npcm_fiu_info *info;
> + struct spi_mem_op drd_op;
> + struct resource *res_mem;
> + struct regmap *regmap;
> + unsigned long clkrate;
> + struct device *dev;
> + struct clk *clk;
> + bool spix_mode;
> +};
> +
> +static const struct regmap_config npcm_mtd_regmap_config = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> + .max_register = NPCM_FIU_MAX_REG_LIMIT,
> +};
> +
> +static void npcm_fiu_set_drd(struct npcm_fiu_spi *fiu,
> + const struct spi_mem_op *op)
> +{
> + regmap_update_bits(fiu->regmap, NPCM_FIU_DRD_CFG,
> + NPCM_FIU_DRD_CFG_ACCTYPE,
> + ilog2(op->addr.buswidth) <<
> + NPCM_FIU_DRD_ACCTYPE_SHIFT);
> + fiu->drd_op.addr.buswidth = op->addr.buswidth;
> + regmap_update_bits(fiu->regmap, NPCM_FIU_DRD_CFG,
> + NPCM_FIU_DRD_CFG_DBW,
> + ((op->dummy.nbytes * ilog2(op->addr.buswidth))
> + / NUM_BITS_IN_BYTE) << NPCM_FIU_DRD_DBW_SHIFT);
> + fiu->drd_op.dummy.nbytes = op->dummy.nbytes;
> + regmap_update_bits(fiu->regmap, NPCM_FIU_DRD_CFG,
> + NPCM_FIU_DRD_CFG_RDCMD, op->cmd.opcode);
> + fiu->drd_op.cmd.opcode = op->cmd.opcode;
> + regmap_update_bits(fiu->regmap, NPCM_FIU_DRD_CFG,
> + NPCM_FIU_DRD_CFG_ADDSIZ,
> + (op->addr.nbytes - 3) << NPCM_FIU_DRD_ADDSIZ_SHIFT);
> + fiu->drd_op.addr.nbytes = op->addr.nbytes;
> +}
> +
> +static ssize_t npcm_fiu_direct_read(struct spi_mem_dirmap_desc *desc,
> + u64 offs, size_t len, void *buf)
> +{
> + struct npcm_fiu_spi *fiu =
> + spi_controller_get_devdata(desc->mem->spi->master);
> + struct npcm_fiu_chip *chip = &fiu->chip[desc->mem->spi->chip_select];
> + void __iomem *src = (void __iomem *)(chip->flash_region_mapped_ptr +
> + offs);
> + u8 *buf_rx = buf;
> + u32 i;
> +
> + if (fiu->spix_mode) {
> + for (i = 0 ; i < len ; i++)
> + *(buf_rx + i) = ioread8(src + i);
> + } else {
> + if (desc->info.op_tmpl.addr.buswidth != fiu->drd_op.addr.buswidth ||
> + desc->info.op_tmpl.dummy.nbytes != fiu->drd_op.dummy.nbytes ||
> + desc->info.op_tmpl.cmd.opcode != fiu->drd_op.cmd.opcode ||
> + desc->info.op_tmpl.addr.nbytes != fiu->drd_op.addr.nbytes)
> + npcm_fiu_set_drd(fiu, &desc->info.op_tmpl);
> +
> + memcpy_fromio(buf_rx, src, len);
Does this need to make sure the memcpy is aligned, or is that handled
at a higher layer?
> + }
> +
> + return len;
> +}
> +
> +static ssize_t npcm_fiu_direct_write(struct spi_mem_dirmap_desc *desc,
> + u64 offs, size_t len, const void *buf)
> +{
> + struct npcm_fiu_spi *fiu =
> + spi_controller_get_devdata(desc->mem->spi->master);
> + struct npcm_fiu_chip *chip = &fiu->chip[desc->mem->spi->chip_select];
> + void __iomem *dst = (void __iomem *)(chip->flash_region_mapped_ptr +
> + offs);
> + const u8 *buf_tx = buf;
> + u32 i;
> +
> + if (fiu->spix_mode)
> + for (i = 0 ; i < len ; i++)
> + iowrite8(*(buf_tx + i), dst + i);
> + else
> + memcpy_toio(dst, buf_tx, len);
> +
> + return len;
> +}
> +
> +static int npcm_fiu_uma_read(struct spi_mem *mem,
> + const struct spi_mem_op *op, u32 addr,
> + bool is_address_size, u8 *data, u32 data_size)
> +{
> + struct npcm_fiu_spi *fiu =
> + spi_controller_get_devdata(mem->spi->master);
> + u32 uma_cfg = BIT(10);
> + u32 data_reg[4];
> + int ret;
> + u32 val;
> + u32 i;
> +
> + regmap_update_bits(fiu->regmap, NPCM_FIU_UMA_CTS,
> + NPCM_FIU_UMA_CTS_DEV_NUM,
> + (mem->spi->chip_select <<
> + NPCM_FIU_UMA_CTS_DEV_NUM_SHIFT));
> + regmap_update_bits(fiu->regmap, NPCM_FIU_UMA_CMD,
> + NPCM_FIU_UMA_CMD_CMD, op->cmd.opcode);
> +
> + if (is_address_size) {
> + uma_cfg |= ilog2(op->cmd.buswidth);
> + uma_cfg |= ilog2(op->addr.buswidth)
> + << NPCM_FIU_UMA_CFG_ADBPCK_SHIFT;
> + uma_cfg |= ilog2(op->dummy.buswidth)
> + << NPCM_FIU_UMA_CFG_DBPCK_SHIFT;
> + uma_cfg |= ilog2(op->data.buswidth)
> + << NPCM_FIU_UMA_CFG_RDBPCK_SHIFT;
> + uma_cfg |= op->dummy.nbytes << NPCM_FIU_UMA_CFG_DBSIZ_SHIFT;
> + uma_cfg |= op->addr.nbytes << NPCM_FIU_UMA_CFG_ADDSIZ_SHIFT;
> + regmap_write(fiu->regmap, NPCM_FIU_UMA_ADDR, addr);
> + } else {
> + regmap_write(fiu->regmap, NPCM_FIU_UMA_ADDR, 0x0);
> + }
> +
> + uma_cfg |= data_size << NPCM_FIU_UMA_CFG_RDATSIZ_SHIFT;
> + regmap_write(fiu->regmap, NPCM_FIU_UMA_CFG, uma_cfg);
> + regmap_write_bits(fiu->regmap, NPCM_FIU_UMA_CTS,
> + NPCM_FIU_UMA_CTS_EXEC_DONE,
> + NPCM_FIU_UMA_CTS_EXEC_DONE);
> + ret = regmap_read_poll_timeout(fiu->regmap, NPCM_FIU_UMA_CTS, val,
> + (!(val & NPCM_FIU_UMA_CTS_EXEC_DONE)), 0,
> + UMA_MICRO_SEC_TIMEOUT);
> + if (ret)
> + return ret;
> +
> + if (data_size) {
> + for (i = 0; i < DIV_ROUND_UP(data_size, 4); i++)
> + regmap_read(fiu->regmap, NPCM_FIU_UMA_DR0 + (i * 4),
> + &data_reg[i]);
> + memcpy(data, data_reg, data_size);
> + }
> +
> + return 0;
> +}
> +
> +static int npcm_fiu_uma_write(struct spi_mem *mem,
> + const struct spi_mem_op *op, u8 cmd,
> + bool is_address_size, u8 *data, u32 data_size)
> +{
> + struct npcm_fiu_spi *fiu =
> + spi_controller_get_devdata(mem->spi->master);
> + u32 uma_cfg = BIT(10);
> + u32 data_reg[4] = {0};
> + u32 val;
> + u32 i;
> +
> + regmap_update_bits(fiu->regmap, NPCM_FIU_UMA_CTS,
> + NPCM_FIU_UMA_CTS_DEV_NUM,
> + (mem->spi->chip_select <<
> + NPCM_FIU_UMA_CTS_DEV_NUM_SHIFT));
> +
> + regmap_update_bits(fiu->regmap, NPCM_FIU_UMA_CMD,
> + NPCM_FIU_UMA_CMD_CMD, cmd);
> +
> + if (data_size) {
> + memcpy(data_reg, data, data_size);
> + for (i = 0; i < DIV_ROUND_UP(data_size, 4); i++)
> + regmap_write(fiu->regmap, NPCM_FIU_UMA_DW0 + (i * 4),
> + data_reg[i]);
> + }
> +
> + if (is_address_size) {
> + uma_cfg |= ilog2(op->cmd.buswidth);
> + uma_cfg |= ilog2(op->addr.buswidth) <<
> + NPCM_FIU_UMA_CFG_ADBPCK_SHIFT;
> + uma_cfg |= ilog2(op->data.buswidth) <<
> + NPCM_FIU_UMA_CFG_WDBPCK_SHIFT;
> + uma_cfg |= op->addr.nbytes << NPCM_FIU_UMA_CFG_ADDSIZ_SHIFT;
> + regmap_write(fiu->regmap, NPCM_FIU_UMA_ADDR, op->addr.val);
> + } else {
> + regmap_write(fiu->regmap, NPCM_FIU_UMA_ADDR, 0x0);
> + }
> +
> + uma_cfg |= (data_size << NPCM_FIU_UMA_CFG_WDATSIZ_SHIFT);
> + regmap_write(fiu->regmap, NPCM_FIU_UMA_CFG, uma_cfg);
> +
> + regmap_write_bits(fiu->regmap, NPCM_FIU_UMA_CTS,
> + NPCM_FIU_UMA_CTS_EXEC_DONE,
> + NPCM_FIU_UMA_CTS_EXEC_DONE);
> +
> + return regmap_read_poll_timeout(fiu->regmap, NPCM_FIU_UMA_CTS, val,
> + (!(val & NPCM_FIU_UMA_CTS_EXEC_DONE)), 0,
> + UMA_MICRO_SEC_TIMEOUT);
> +}
> +
> +static int npcm_fiu_manualwrite(struct spi_mem *mem,
> + const struct spi_mem_op *op)
> +{
> + struct npcm_fiu_spi *fiu =
> + spi_controller_get_devdata(mem->spi->master);
> + u8 *data = (u8 *)op->data.buf.out;
> + u32 num_data_chunks;
> + u32 remain_data;
> + u32 idx = 0;
> + int ret;
> +
> + num_data_chunks = op->data.nbytes / CHUNK_SIZE;
> + remain_data = op->data.nbytes % CHUNK_SIZE;
> +
> + regmap_update_bits(fiu->regmap, NPCM_FIU_UMA_CTS,
> + NPCM_FIU_UMA_CTS_DEV_NUM,
> + (mem->spi->chip_select <<
> + NPCM_FIU_UMA_CTS_DEV_NUM_SHIFT));
> + regmap_update_bits(fiu->regmap, NPCM_FIU_UMA_CTS,
> + NPCM_FIU_UMA_CTS_SW_CS, 0);
> +
> + ret = npcm_fiu_uma_write(mem, op, op->cmd.opcode, true, NULL, 0);
> + if (ret)
> + return ret;
> +
> + /* Starting the data writing loop in multiples of 8 */
> + for (idx = 0; idx < num_data_chunks; ++idx) {
> + ret = npcm_fiu_uma_write(mem, op, data[0], false,
> + &data[1], CHUNK_SIZE - 1);
> + if (ret)
> + return ret;
> +
> + data += CHUNK_SIZE;
> + }
> +
> + /* Handling chunk remains */
> + if (remain_data > 0) {
> + ret = npcm_fiu_uma_write(mem, op, data[0], false,
> + &data[1], remain_data - 1);
> + if (ret)
> + return ret;
> + }
> +
> + regmap_update_bits(fiu->regmap, NPCM_FIU_UMA_CTS,
> + NPCM_FIU_UMA_CTS_SW_CS, NPCM_FIU_UMA_CTS_SW_CS);
> +
> + return 0;
> +}
> +
> +static int npcm_fiu_read(struct spi_mem *mem, const struct spi_mem_op *op)
> +{
> + u8 *data = op->data.buf.in;
> + int i, readlen, currlen;
> + size_t retlen = 0;
> + u8 *buf_ptr;
> + u32 addr;
> + int ret;
> +
> + i = 0;
> + currlen = op->data.nbytes;
> +
> + do {
> + addr = ((u32)op->addr.val + i);
> + if (currlen < 16)
> + readlen = currlen;
> + else
> + readlen = 16;
> +
> + buf_ptr = data + i;
> + ret = npcm_fiu_uma_read(mem, op, addr, true, buf_ptr,
> + readlen);
> + if (ret)
> + return ret;
> +
> + i += readlen;
> + currlen -= 16;
> + } while (currlen > 0);
> +
> + retlen = i;
> +
> + return 0;
> +}
> +
> +static void npcm_fiux_set_direct_wr(struct npcm_fiu_spi *fiu)
> +{
> + regmap_write(fiu->regmap, NPCM_FIU_DWR_CFG,
> + NPCM_FIU_DWR_16_BYTE_BURST);
> + regmap_update_bits(fiu->regmap, NPCM_FIU_DWR_CFG,
> + NPCM_FIU_DWR_CFG_ABPCK,
> + DWR_ABPCK_4_BIT_PER_CLK << NPCM_FIU_DWR_ABPCK_SHIFT);
> + regmap_update_bits(fiu->regmap, NPCM_FIU_DWR_CFG,
> + NPCM_FIU_DWR_CFG_DBPCK,
> + DWR_DBPCK_4_BIT_PER_CLK << NPCM_FIU_DWR_DBPCK_SHIFT);
> +}
> +
> +static void npcm_fiux_set_direct_rd(struct npcm_fiu_spi *fiu)
> +{
> + u32 rx_dummy = 0;
> +
> + regmap_write(fiu->regmap, NPCM_FIU_DRD_CFG,
> + NPCM_FIU_DRD_16_BYTE_BURST);
> + regmap_update_bits(fiu->regmap, NPCM_FIU_DRD_CFG,
> + NPCM_FIU_DRD_CFG_ACCTYPE,
> + DRD_SPI_X_MODE << NPCM_FIU_DRD_ACCTYPE_SHIFT);
> + regmap_update_bits(fiu->regmap, NPCM_FIU_DRD_CFG,
> + NPCM_FIU_DRD_CFG_DBW,
> + rx_dummy << NPCM_FIU_DRD_DBW_SHIFT);
> +}
> +
> +static int npcm_fiu_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
> +{
> + struct npcm_fiu_spi *fiu =
> + spi_controller_get_devdata(mem->spi->master);
> + struct npcm_fiu_chip *chip = &fiu->chip[mem->spi->chip_select];
> + int ret = 0;
> + u8 *buf;
> +
> + dev_dbg(fiu->dev, "cmd:%#x mode:%d.%d.%d.%d addr:%#llx len:%#x\n",
> + op->cmd.opcode, op->cmd.buswidth, op->addr.buswidth,
> + op->dummy.buswidth, op->data.buswidth, op->addr.val,
> + op->data.nbytes);
> +
> + if (fiu->spix_mode)
> + return -ENOTSUPP;
> +
> + if (fiu->clkrate != chip->clkrate) {
> + ret = clk_set_rate(fiu->clk, chip->clkrate);
> + if (ret < 0)
> + dev_warn(fiu->dev, "Failed setting %lu frequancy, stay at %lu frequancy\n", chip->clkrate, fiu->clkrate);
> + else
> + fiu->clkrate = chip->clkrate;
> + }
> +
> + if (op->data.dir == SPI_MEM_DATA_IN) {
> + if (!op->addr.nbytes) {
> + buf = op->data.buf.in;
> + ret = npcm_fiu_uma_read(mem, op, op->addr.val, false,
> + buf, op->data.nbytes);
> + } else {
> + ret = npcm_fiu_read(mem, op);
> + }
> + } else {
> + if (!op->addr.nbytes || !op->data.nbytes) {
> + if (op->data.nbytes)
> + buf = (u8 *)op->data.buf.out;
> + else
> + buf = NULL;
> + ret = npcm_fiu_uma_write(mem, op, op->cmd.opcode, false,
> + buf, op->data.nbytes);
> + } else {
> + ret = npcm_fiu_manualwrite(mem, op);
> + }
> + }
> +
> + return ret;
> +}
> +
> +static int npcm_fiu_dirmap_create(struct spi_mem_dirmap_desc *desc)
> +{
> + struct npcm_fiu_spi *fiu =
> + spi_controller_get_devdata(desc->mem->spi->master);
> + struct npcm_fiu_chip *chip = &fiu->chip[desc->mem->spi->chip_select];
> + struct regmap *gcr_regmap;
> +
> + if (!fiu->res_mem) {
> + dev_warn(fiu->dev, "Reserved memory not defined, direct read disabled\n");
> + desc->nodirmap = true;
> + return 0;
> + }
> +
> + if (!fiu->spix_mode &&
> + desc->info.op_tmpl.data.dir == SPI_MEM_DATA_OUT) {
> + desc->nodirmap = true;
> + return 0;
> + }
> +
> + if (!chip->flash_region_mapped_ptr) {
> + chip->flash_region_mapped_ptr =
> + devm_ioremap_nocache(fiu->dev, (fiu->res_mem->start +
> + (fiu->info->max_map_size *
> + desc->mem->spi->chip_select)),
> + (u32)desc->info.length);
> + if (!chip->flash_region_mapped_ptr) {
> + dev_warn(fiu->dev, "Error mapping memory region, direct read disabled\n");
> + desc->nodirmap = true;
> + return 0;
> + }
> + }
> +
> + if (of_device_is_compatible(fiu->dev->of_node, "nuvoton,npcm750-fiu")) {
> + gcr_regmap =
> + syscon_regmap_lookup_by_compatible("nuvoton,npcm750-gcr");
> + if (IS_ERR(gcr_regmap)) {
> + dev_warn(fiu->dev, "Didn't find nuvoton,npcm750-gcr, direct read disabled\n");
> + desc->nodirmap = true;
> + return 0;
> + }
> + regmap_update_bits(gcr_regmap, NPCM7XX_INTCR3_OFFSET,
> + NPCM7XX_INTCR3_FIU_FIX,
> + NPCM7XX_INTCR3_FIU_FIX);
> + }
> +
> + if (desc->info.op_tmpl.data.dir == SPI_MEM_DATA_IN) {
> + if (!fiu->spix_mode)
> + npcm_fiu_set_drd(fiu, &desc->info.op_tmpl);
> + else
> + npcm_fiux_set_direct_rd(fiu);
> +
> + } else {
> + npcm_fiux_set_direct_wr(fiu);
> + }
> +
> + return 0;
> +}
> +
> +static int npcm_fiu_setup(struct spi_device *spi)
> +{
> + struct spi_controller *ctrl = spi->master;
> + struct npcm_fiu_spi *fiu = spi_controller_get_devdata(ctrl);
> + struct npcm_fiu_chip *chip;
> +
> + chip = &fiu->chip[spi->chip_select];
> + chip->fiu = fiu;
> + chip->chipselect = spi->chip_select;
> + chip->clkrate = spi->max_speed_hz;
> +
> + fiu->clkrate = clk_get_rate(fiu->clk);
> +
> + return 0;
> +}
> +
> +static const struct spi_controller_mem_ops npcm_fiu_mem_ops = {
> + .exec_op = npcm_fiu_exec_op,
> + .dirmap_create = npcm_fiu_dirmap_create,
> + .dirmap_read = npcm_fiu_direct_read,
> + .dirmap_write = npcm_fiu_direct_write,
> +};
> +
> +static const struct of_device_id npcm_fiu_dt_ids[] = {
> + { .compatible = "nuvoton,npcm750-fiu", .data = &npxm7xx_fiu_data },
> + { /* sentinel */ }
> +};
> +
> +static int npcm_fiu_probe(struct platform_device *pdev)
> +{
> + const struct fiu_data *fiu_data_match;
> + const struct of_device_id *match;
> + struct device *dev = &pdev->dev;
> + struct spi_controller *ctrl;
> + struct npcm_fiu_spi *fiu;
> + void __iomem *regbase;
> + struct resource *res;
> + int ret;
> + int id;
> +
> + ctrl = spi_alloc_master(dev, sizeof(*fiu));
> + if (!ctrl)
> + return -ENOMEM;
> +
> + fiu = spi_controller_get_devdata(ctrl);
> +
> + match = of_match_device(npcm_fiu_dt_ids, dev);
> + if (!match || !match->data) {
> + dev_err(dev, "No compatible OF match\n");
> + return -ENODEV;
> + }
> +
> + fiu_data_match = match->data;
> + id = of_alias_get_id(dev->of_node, "fiu");
> + if (id < 0 || id >= fiu_data_match->fiu_max) {
> + dev_err(dev, "Invalid platform device id: %d\n", id);
> + return -EINVAL;
> + }
> +
> + fiu->info = &fiu_data_match->npcm_fiu_data_info[id];
> +
> + platform_set_drvdata(pdev, fiu);
> + fiu->dev = dev;
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "control");
> + regbase = devm_ioremap_resource(dev, res);
> + if (IS_ERR(regbase))
> + return PTR_ERR(regbase);
> +
> + fiu->regmap = devm_regmap_init_mmio(dev, regbase,
> + &npcm_mtd_regmap_config);
> + if (IS_ERR(fiu->regmap)) {
> + dev_err(dev, "Failed to create regmap\n");
> + return PTR_ERR(fiu->regmap);
> + }
> +
> + fiu->res_mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> + "memory");
> + fiu->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(fiu->clk))
> + return PTR_ERR(fiu->clk);
> +
> + fiu->spix_mode = of_property_read_bool(dev->of_node, "spix-mode");
> +
> + platform_set_drvdata(pdev, fiu);
> + clk_prepare_enable(fiu->clk);
> +
> + ctrl->mode_bits = SPI_RX_DUAL | SPI_RX_QUAD
> + | SPI_TX_DUAL | SPI_TX_QUAD;
> + ctrl->setup = npcm_fiu_setup;
> + ctrl->bus_num = -1;
> + ctrl->mem_ops = &npcm_fiu_mem_ops;
> + ctrl->num_chipselect = fiu->info->max_cs;
> + ctrl->dev.of_node = dev->of_node;
> +
> + ret = devm_spi_register_master(dev, ctrl);
> + if (ret)
> + return ret;
> +
> + dev_info(dev, "NPCM %s probe succeed\n", fiu->info->name);
> +
> + return 0;
> +}
> +
> +static int npcm_fiu_remove(struct platform_device *pdev)
> +{
> + struct npcm_fiu_spi *fiu = platform_get_drvdata(pdev);
> +
> + clk_disable_unprepare(fiu->clk);
> + return 0;
> +}
> +
> +MODULE_DEVICE_TABLE(of, npcm_fiu_dt_ids);
> +
> +static struct platform_driver npcm_fiu_driver = {
> + .driver = {
> + .name = "NPCM-FIU",
> + .bus = &platform_bus_type,
> + .of_match_table = npcm_fiu_dt_ids,
> + },
> + .probe = npcm_fiu_probe,
> + .remove = npcm_fiu_remove,
> +};
> +module_platform_driver(npcm_fiu_driver);
> +
> +MODULE_DESCRIPTION("Nuvoton FLASH Interface Unit SPI Controller Driver");
> +MODULE_AUTHOR("Tomer Maimon <tomer.maimon@nuvoton.com>");
> +MODULE_LICENSE("GPL v2");
> --
> 2.18.0
>
^ permalink raw reply
* Re: [PATCH v8 11/21] clk: tegra: clk-dfll: Add suspend and resume support
From: Dmitry Osipenko @ 2019-08-09 18:00 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree, rjw, viresh.kumar, linux-pm
In-Reply-To: <29a85a35-10ff-2d43-d148-9dba1ee25869@nvidia.com>
09.08.2019 19:39, Sowjanya Komatineni пишет:
>
> On 8/9/19 5:23 AM, Dmitry Osipenko wrote:
>> 09.08.2019 2:46, Sowjanya Komatineni пишет:
>>> This patch implements DFLL suspend and resume operation.
>>>
>>> During system suspend entry, CPU clock will switch CPU to safe
>>> clock source of PLLP and disables DFLL clock output.
>>>
>>> DFLL driver suspend confirms DFLL disable state and errors out on
>>> being active.
>>>
>>> DFLL is re-initialized during the DFLL driver resume as it goes
>>> through complete reset during suspend entry.
>>>
>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>> ---
>>> drivers/clk/tegra/clk-dfll.c | 56 ++++++++++++++++++++++++++++++
>>> drivers/clk/tegra/clk-dfll.h | 2 ++
>>> drivers/clk/tegra/clk-tegra124-dfll-fcpu.c | 1 +
>>> 3 files changed, 59 insertions(+)
>>>
>>> diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
>>> index f8688c2ddf1a..eb298a5d7be9 100644
>>> --- a/drivers/clk/tegra/clk-dfll.c
>>> +++ b/drivers/clk/tegra/clk-dfll.c
>>> @@ -1487,6 +1487,7 @@ static int dfll_init(struct tegra_dfll *td)
>>> td->last_unrounded_rate = 0;
>>> pm_runtime_enable(td->dev);
>>> + pm_runtime_irq_safe(td->dev);
>>> pm_runtime_get_sync(td->dev);
>>> dfll_set_mode(td, DFLL_DISABLED);
>>> @@ -1513,6 +1514,61 @@ static int dfll_init(struct tegra_dfll *td)
>>> return ret;
>>> }
>>> +/**
>>> + * tegra_dfll_suspend - check DFLL is disabled
>>> + * @dev: DFLL device *
>>> + *
>>> + * DFLL clock should be disabled by the CPUFreq driver. So, make
>>> + * sure it is disabled and disable all clocks needed by the DFLL.
>>> + */
>>> +int tegra_dfll_suspend(struct device *dev)
>>> +{
>>> + struct tegra_dfll *td = dev_get_drvdata(dev);
>>> +
>>> + if (dfll_is_running(td)) {
>>> + dev_err(td->dev, "dfll is enabled while shouldn't be\n");
>>> + return -EBUSY;
>>> + }
>>> +
>>> + reset_control_assert(td->dvco_rst);
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL(tegra_dfll_suspend);
>>> +
>>> +/**
>>> + * tegra_dfll_resume - reinitialize DFLL on resume
>>> + * @dev: DFLL instance
>>> + *
>>> + * DFLL is disabled and reset during suspend and resume.
>>> + * So, reinitialize the DFLL IP block back for use.
>>> + * DFLL clock is enabled later in closed loop mode by CPUFreq
>>> + * driver before switching its clock source to DFLL output.
>>> + */
>>> +int tegra_dfll_resume(struct device *dev)
>>> +{
>>> + struct tegra_dfll *td = dev_get_drvdata(dev);
>>> +
>>> + reset_control_deassert(td->dvco_rst);
>> This doesn't look right because I assume that DFLL resetting is
>> synchronous and thus clk should be enabled in order for reset to
>> propagate inside hardware.
>>
>>> + pm_runtime_get_sync(td->dev);
>> Hence it will be better to remove the above reset_control_deassert() and
>> add here:
>>
>> reset_control_reset(td->dvco_rst);
>
> By the time dfll resume happens, dfll controller clock will already be enabled.
>
> so doing reset de-assert before pm_runtime seems ok.
I don't see what enables the DFLL clock because it should be enabled by the CPUFreq driver
on resume from suspend and resume happens after resuming of the DFLL driver.
^ permalink raw reply
* Re: [PATCH 4/4] regulator: qcom-rpmh: Update PMIC modes for PMIC5
From: Bjorn Andersson @ 2019-08-09 18:00 UTC (permalink / raw)
To: Vinod Koul
Cc: Mark Brown, linux-arm-msm, Andy Gross, Liam Girdwood, Rob Herring,
Mark Rutland, linux-kernel, devicetree
In-Reply-To: <20190809073616.1235-4-vkoul@kernel.org>
On Fri 09 Aug 00:36 PDT 2019, Vinod Koul wrote:
> Add the PMIC5 modes and use them pmic5 ldo and smps
>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> drivers/regulator/qcom-rpmh-regulator.c | 52 +++++++++++++++++++++----
> 1 file changed, 45 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
> index 391ed844a251..db6c085da65e 100644
> --- a/drivers/regulator/qcom-rpmh-regulator.c
> +++ b/drivers/regulator/qcom-rpmh-regulator.c
> @@ -50,6 +50,20 @@ enum rpmh_regulator_type {
> #define PMIC4_BOB_MODE_AUTO 2
> #define PMIC4_BOB_MODE_PWM 3
>
> +#define PMIC5_LDO_MODE_RETENTION 3
> +#define PMIC5_LDO_MODE_LPM 4
> +#define PMIC5_LDO_MODE_HPM 7
> +
> +#define PMIC5_SMPS_MODE_RETENTION 3
> +#define PMIC5_SMPS_MODE_PFM 4
> +#define PMIC5_SMPS_MODE_AUTO 6
> +#define PMIC5_SMPS_MODE_PWM 7
> +
> +#define PMIC5_BOB_MODE_PASS 2
> +#define PMIC5_BOB_MODE_PFM 4
> +#define PMIC5_BOB_MODE_AUTO 6
> +#define PMIC5_BOB_MODE_PWM 7
> +
> /**
> * struct rpmh_vreg_hw_data - RPMh regulator hardware configurations
> * @regulator_type: RPMh accelerator type used to manage this
> @@ -488,6 +502,14 @@ static const int pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
> [REGULATOR_MODE_FAST] = -EINVAL,
> };
>
> +static const int pmic_mode_map_pmic5_ldo[REGULATOR_MODE_STANDBY + 1] = {
> + [REGULATOR_MODE_INVALID] = -EINVAL,
> + [REGULATOR_MODE_STANDBY] = PMIC5_LDO_MODE_RETENTION,
> + [REGULATOR_MODE_IDLE] = PMIC5_LDO_MODE_LPM,
> + [REGULATOR_MODE_NORMAL] = PMIC5_LDO_MODE_HPM,
> + [REGULATOR_MODE_FAST] = -EINVAL,
> +};
> +
> static unsigned int rpmh_regulator_pmic4_ldo_of_map_mode(unsigned int rpmh_mode)
> {
> unsigned int mode;
> @@ -518,6 +540,14 @@ static const int pmic_mode_map_pmic4_smps[REGULATOR_MODE_STANDBY + 1] = {
> [REGULATOR_MODE_FAST] = PMIC4_SMPS_MODE_PWM,
> };
>
> +static const int pmic_mode_map_pmic5_smps[REGULATOR_MODE_STANDBY + 1] = {
> + [REGULATOR_MODE_INVALID] = -EINVAL,
> + [REGULATOR_MODE_STANDBY] = PMIC5_SMPS_MODE_RETENTION,
> + [REGULATOR_MODE_IDLE] = PMIC5_SMPS_MODE_PFM,
> + [REGULATOR_MODE_NORMAL] = PMIC5_SMPS_MODE_AUTO,
> + [REGULATOR_MODE_FAST] = PMIC5_SMPS_MODE_PWM,
> +};
> +
> static unsigned int
> rpmh_regulator_pmic4_smps_of_map_mode(unsigned int rpmh_mode)
> {
> @@ -552,6 +582,14 @@ static const int pmic_mode_map_pmic4_bob[REGULATOR_MODE_STANDBY + 1] = {
> [REGULATOR_MODE_FAST] = PMIC4_BOB_MODE_PWM,
> };
>
> +static const int pmic_mode_map_pmic5_bob[REGULATOR_MODE_STANDBY + 1] = {
> + [REGULATOR_MODE_INVALID] = -EINVAL,
> + [REGULATOR_MODE_STANDBY] = -EINVAL,
> + [REGULATOR_MODE_IDLE] = PMIC5_BOB_MODE_PFM,
> + [REGULATOR_MODE_NORMAL] = PMIC5_BOB_MODE_AUTO,
> + [REGULATOR_MODE_FAST] = PMIC5_BOB_MODE_PWM,
> +};
> +
> static unsigned int rpmh_regulator_pmic4_bob_of_map_mode(unsigned int rpmh_mode)
> {
> unsigned int mode;
> @@ -643,7 +681,7 @@ static const struct rpmh_vreg_hw_data pmic5_pldo = {
> .voltage_range = REGULATOR_LINEAR_RANGE(1504000, 0, 255, 8000),
> .n_voltages = 256,
> .hpm_min_load_uA = 10000,
> - .pmic_mode_map = pmic_mode_map_pmic4_ldo,
> + .pmic_mode_map = pmic_mode_map_pmic5_ldo,
> .of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
> };
>
> @@ -653,7 +691,7 @@ static const struct rpmh_vreg_hw_data pmic5_pldo_lv = {
> .voltage_range = REGULATOR_LINEAR_RANGE(1504000, 0, 62, 8000),
> .n_voltages = 63,
> .hpm_min_load_uA = 10000,
> - .pmic_mode_map = pmic_mode_map_pmic4_ldo,
> + .pmic_mode_map = pmic_mode_map_pmic5_ldo,
> .of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
> };
>
> @@ -663,7 +701,7 @@ static const struct rpmh_vreg_hw_data pmic5_nldo = {
> .voltage_range = REGULATOR_LINEAR_RANGE(320000, 0, 123, 8000),
> .n_voltages = 124,
> .hpm_min_load_uA = 30000,
> - .pmic_mode_map = pmic_mode_map_pmic4_ldo,
> + .pmic_mode_map = pmic_mode_map_pmic5_ldo,
> .of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
> };
>
> @@ -672,7 +710,7 @@ static const struct rpmh_vreg_hw_data pmic5_hfsmps510 = {
> .ops = &rpmh_regulator_vrm_ops,
> .voltage_range = REGULATOR_LINEAR_RANGE(320000, 0, 215, 8000),
> .n_voltages = 216,
> - .pmic_mode_map = pmic_mode_map_pmic4_smps,
> + .pmic_mode_map = pmic_mode_map_pmic5_smps,
> .of_map_mode = rpmh_regulator_pmic4_smps_of_map_mode,
> };
>
> @@ -681,7 +719,7 @@ static const struct rpmh_vreg_hw_data pmic5_ftsmps510 = {
> .ops = &rpmh_regulator_vrm_ops,
> .voltage_range = REGULATOR_LINEAR_RANGE(300000, 0, 263, 4000),
> .n_voltages = 264,
> - .pmic_mode_map = pmic_mode_map_pmic4_smps,
> + .pmic_mode_map = pmic_mode_map_pmic5_smps,
> .of_map_mode = rpmh_regulator_pmic4_smps_of_map_mode,
> };
>
> @@ -690,7 +728,7 @@ static const struct rpmh_vreg_hw_data pmic5_hfsmps515 = {
> .ops = &rpmh_regulator_vrm_ops,
> .voltage_range = REGULATOR_LINEAR_RANGE(2800000, 0, 4, 1600),
> .n_voltages = 5,
> - .pmic_mode_map = pmic_mode_map_pmic4_smps,
> + .pmic_mode_map = pmic_mode_map_pmic5_smps,
> .of_map_mode = rpmh_regulator_pmic4_smps_of_map_mode,
> };
>
> @@ -699,7 +737,7 @@ static const struct rpmh_vreg_hw_data pmic5_bob = {
> .ops = &rpmh_regulator_vrm_bypass_ops,
> .voltage_range = REGULATOR_LINEAR_RANGE(300000, 0, 135, 32000),
> .n_voltages = 136,
> - .pmic_mode_map = pmic_mode_map_pmic4_bob,
> + .pmic_mode_map = pmic_mode_map_pmic5_bob,
> .of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
> };
>
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH v8 05/21] clk: tegra: pll: Save and restore pll context
From: Dmitry Osipenko @ 2019-08-09 17:50 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree, rjw, viresh.kumar, linux-pm
In-Reply-To: <dd20aa34-d838-40c4-9edd-bbe5973053f3@nvidia.com>
09.08.2019 20:39, Sowjanya Komatineni пишет:
>
> On 8/9/19 4:33 AM, Dmitry Osipenko wrote:
>> 09.08.2019 2:46, Sowjanya Komatineni пишет:
>>> This patch implements save and restore of PLL context.
>>>
>>> During system suspend, core power goes off and looses the settings
>>> of the Tegra CAR controller registers.
>>>
>>> So during suspend entry pll context is stored and on resume it is
>>> restored back along with its state.
>>>
>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>> ---
>>> drivers/clk/tegra/clk-pll.c | 88 ++++++++++++++++++++++++++++-----------------
>>> drivers/clk/tegra/clk.h | 2 ++
>>> 2 files changed, 58 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
>>> index 1583f5fc992f..e52add2bbdbb 100644
>>> --- a/drivers/clk/tegra/clk-pll.c
>>> +++ b/drivers/clk/tegra/clk-pll.c
>>> @@ -1008,6 +1008,28 @@ static unsigned long clk_plle_recalc_rate(struct clk_hw *hw,
>>> return rate;
>>> }
>>> +static void tegra_clk_pll_restore_context(struct clk_hw *hw)
>>> +{
>>> + struct tegra_clk_pll *pll = to_clk_pll(hw);
>>> + struct clk_hw *parent = clk_hw_get_parent(hw);
>>> + unsigned long parent_rate = clk_hw_get_rate(parent);
>>> + unsigned long rate = clk_hw_get_rate(hw);
>>> + u32 val;
>>> +
>>> + if (clk_pll_is_enabled(hw))
>>> + return;
>>> +
>>> + if (pll->params->set_defaults)
>>> + pll->params->set_defaults(pll);
>>> +
>>> + clk_pll_set_rate(hw, rate, parent_rate);
>>> +
>>> + if (!__clk_get_enable_count(hw->clk))
>> What about orphaned clocks? Is enable_count > 0 for them?
> There are no orphaned pll clocks.
Sorry, I meant the "clk_ignore_unused".
^ permalink raw reply
* Re: [PATCH v8 05/21] clk: tegra: pll: Save and restore pll context
From: Sowjanya Komatineni @ 2019-08-09 17:39 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree, rjw, viresh.kumar, linux-pm
In-Reply-To: <68f65db6-44b7-1c75-2633-4a2fffd62a92@gmail.com>
On 8/9/19 4:33 AM, Dmitry Osipenko wrote:
> 09.08.2019 2:46, Sowjanya Komatineni пишет:
>> This patch implements save and restore of PLL context.
>>
>> During system suspend, core power goes off and looses the settings
>> of the Tegra CAR controller registers.
>>
>> So during suspend entry pll context is stored and on resume it is
>> restored back along with its state.
>>
>> Acked-by: Thierry Reding <treding@nvidia.com>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
>> drivers/clk/tegra/clk-pll.c | 88 ++++++++++++++++++++++++++++-----------------
>> drivers/clk/tegra/clk.h | 2 ++
>> 2 files changed, 58 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
>> index 1583f5fc992f..e52add2bbdbb 100644
>> --- a/drivers/clk/tegra/clk-pll.c
>> +++ b/drivers/clk/tegra/clk-pll.c
>> @@ -1008,6 +1008,28 @@ static unsigned long clk_plle_recalc_rate(struct clk_hw *hw,
>> return rate;
>> }
>>
>> +static void tegra_clk_pll_restore_context(struct clk_hw *hw)
>> +{
>> + struct tegra_clk_pll *pll = to_clk_pll(hw);
>> + struct clk_hw *parent = clk_hw_get_parent(hw);
>> + unsigned long parent_rate = clk_hw_get_rate(parent);
>> + unsigned long rate = clk_hw_get_rate(hw);
>> + u32 val;
>> +
>> + if (clk_pll_is_enabled(hw))
>> + return;
>> +
>> + if (pll->params->set_defaults)
>> + pll->params->set_defaults(pll);
>> +
>> + clk_pll_set_rate(hw, rate, parent_rate);
>> +
>> + if (!__clk_get_enable_count(hw->clk))
> What about orphaned clocks? Is enable_count > 0 for them?
There are no orphaned pll clocks.
>> + clk_pll_disable(hw);
>> + else
>> + clk_pll_enable(hw);
>> +}
> [snip]
^ permalink raw reply
* [PATCH] of: Fix of_empty_ranges_quirk()
From: marek.vasut @ 2019-08-09 17:33 UTC (permalink / raw)
To: devicetree; +Cc: Marek Vasut, Rob Herring, Frank Rowand, linux-renesas-soc
From: Marek Vasut <marek.vasut+renesas@gmail.com>
The of_empty_ranges_quirk() returns a mix of boolean and signed integer
types, which cannot work well. Replace that with boolean only and fix
usage logic in of_translate_one() -- the check should trigger when the
ranges are NULL and the quirk is applicable on the hardware.
Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: linux-renesas-soc@vger.kernel.org
To: devicetree@vger.kernel.org
---
drivers/of/address.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/of/address.c b/drivers/of/address.c
index b492176c0572..ae2819e148b8 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -616,7 +616,7 @@ static struct of_bus *of_match_bus(struct device_node *np)
return NULL;
}
-static int of_empty_ranges_quirk(struct device_node *np)
+static bool of_empty_ranges_quirk(struct device_node *np)
{
if (IS_ENABLED(CONFIG_PPC)) {
/* To save cycles, we cache the result for global "Mac" setting */
@@ -631,7 +631,8 @@ static int of_empty_ranges_quirk(struct device_node *np)
quirk_state =
of_machine_is_compatible("Power Macintosh") ||
of_machine_is_compatible("MacRISC");
- return quirk_state;
+ if (quirk_state > 0)
+ return true;
}
return false;
}
@@ -662,8 +663,8 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
* This code is only enabled on powerpc. --gcl
*/
ranges = of_get_property(parent, rprop, &rlen);
- if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
- pr_debug("no ranges; cannot translate\n");
+ if (ranges == NULL && of_empty_ranges_quirk(parent)) {
+ pr_err("no ranges; cannot translate\n");
return 1;
}
if (ranges == NULL || rlen == 0) {
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v8 19/21] soc/tegra: pmc: Configure deep sleep control settings
From: Sowjanya Komatineni @ 2019-08-09 17:24 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree, rjw, viresh.kumar, linux-pm
In-Reply-To: <275f3685-bc53-38ff-c778-cf2ea588e5a5@nvidia.com>
On 8/9/19 9:23 AM, Sowjanya Komatineni wrote:
>
> On 8/9/19 6:23 AM, Dmitry Osipenko wrote:
>> 09.08.2019 2:46, Sowjanya Komatineni пишет:
>>> Tegra210 and prior Tegra chips have deep sleep entry and wakeup related
>>> timings which are platform specific that should be configured before
>>> entering into deep sleep.
>>>
>>> Below are the timing specific configurations for deep sleep entry and
>>> wakeup.
>>> - Core rail power-on stabilization timer
>>> - OSC clock stabilization timer after SOC rail power is stabilized.
>>> - Core power off time is the minimum wake delay to keep the system
>>> in deep sleep state irrespective of any quick wake event.
>>>
>>> These values depends on the discharge time of regulators and turn OFF
>>> time of the PMIC to allow the complete system to finish entering into
>>> deep sleep state.
>>>
>>> These values vary based on the platform design and are specified
>>> through the device tree.
>>>
>>> This patch has implementation to configure these timings which are must
>>> to have for proper deep sleep and wakeup operations.
>>>
>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>> ---
>>> drivers/soc/tegra/pmc.c | 13 ++++++++++++-
>>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>>> index e013ada7e4e9..9a78d8417367 100644
>>> --- a/drivers/soc/tegra/pmc.c
>>> +++ b/drivers/soc/tegra/pmc.c
>>> @@ -88,6 +88,8 @@
>>> #define PMC_CPUPWRGOOD_TIMER 0xc8
>>> #define PMC_CPUPWROFF_TIMER 0xcc
>>> +#define PMC_COREPWRGOOD_TIMER 0x3c
>>> +#define PMC_COREPWROFF_TIMER 0xe0
>>> #define PMC_PWR_DET_VALUE 0xe4
>>> @@ -2277,7 +2279,7 @@ static const struct tegra_pmc_regs
>>> tegra20_pmc_regs = {
>>> static void tegra20_pmc_init(struct tegra_pmc *pmc)
>>> {
>>> - u32 value;
>>> + u32 value, osc, pmu, off;
>>> /* Always enable CPU power request */
>>> value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>> @@ -2303,6 +2305,15 @@ static void tegra20_pmc_init(struct tegra_pmc
>>> *pmc)
>>> value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>> value |= PMC_CNTRL_SYSCLK_OE;
>>> tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>> +
>>> + osc = DIV_ROUND_UP(pmc->core_osc_time * 8192, 1000000);
>>> + pmu = DIV_ROUND_UP(pmc->core_pmu_time * 32768, 1000000);
>>> + off = DIV_ROUND_UP(pmc->core_off_time * 32768, 1000000);
>>> + if (osc && pmu)
>>> + tegra_pmc_writel(pmc, ((osc << 8) & 0xff00) | (pmu & 0xff),
>>> + PMC_COREPWRGOOD_TIMER);
>>> + if (off)
>>> + tegra_pmc_writel(pmc, off, PMC_COREPWROFF_TIMER);
>> The osc/pmu/off values are undefined if they are not defined in
>> device-tree. I suppose this
>> need to be corrected in tegra_pmc_parse_dt() if the values really
>> matter even if LP0 suspend
>> isn't supported in device-tree.
>>
>> And I'm also not sure what's wrong with setting 0 for the timers.
>>
> These settings are for SC7 only and will not have any impact in normal
> state.
POR value for these timing registers is not 0 and has default timings
based on chip design and on top of that based on platform HW components
charge/discharge timings there's a need to increase these timings so
support for programming these thru DT is needed and these values have
effect only in LP0.
>>> }
>>> static void tegra20_pmc_setup_irq_polarity(struct tegra_pmc *pmc,
>>>
^ permalink raw reply
* Re: [PATCH v8 10/21] clk: tegra: clk-super: Add restore-context support
From: Sowjanya Komatineni @ 2019-08-09 17:08 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree, rjw, viresh.kumar, linux-pm
In-Reply-To: <4e33bad9-8d5a-dcd7-c75e-db5843c9be4a@gmail.com>
On 8/9/19 5:17 AM, Dmitry Osipenko wrote:
> 09.08.2019 2:46, Sowjanya Komatineni пишет:
>> This patch implements restore_context for clk_super_mux and clk_super.
>>
>> During system supend, core power goes off the and context of Tegra
>> CAR registers is lost.
>>
>> So on system resume, context of super clock registers are restored
>> to have them in same state as before suspend.
>>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
>> drivers/clk/tegra/clk-super.c | 21 +++++++++++++++++++++
>> 1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/clk/tegra/clk-super.c b/drivers/clk/tegra/clk-super.c
>> index e2a1e95a8db7..74c9e913e41c 100644
>> --- a/drivers/clk/tegra/clk-super.c
>> +++ b/drivers/clk/tegra/clk-super.c
>> @@ -124,9 +124,18 @@ static int clk_super_set_parent(struct clk_hw *hw, u8 index)
>> return err;
>> }
>>
>> +static void clk_super_mux_restore_context(struct clk_hw *hw)
>> +{
>> + struct clk_hw *parent = clk_hw_get_parent(hw);
>> + int parent_id = clk_hw_get_parent_index(hw, parent);
>> +
>> + clk_super_set_parent(hw, parent_id);
> All Super clocks have a divider, including the "MUX". Thus I'm wondering
> if there is a chance that divider's configuration may differ on resume
> from what it was on suspend.
tegra_clk_register_super_mux which uses tegra_clk_super_mux_ops doesn't
do divider rate programming.
I believe you are referring to sclk_divider, cclklp_divider,
cclkg_divider...
these are registered as clk_divider and are restored during clk_divider
resume.
>> +}
>> +
>> static const struct clk_ops tegra_clk_super_mux_ops = {
>> .get_parent = clk_super_get_parent,
>> .set_parent = clk_super_set_parent,
>> + .restore_context = clk_super_mux_restore_context,
>> };
>>
>> static long clk_super_round_rate(struct clk_hw *hw, unsigned long rate,
>> @@ -162,12 +171,24 @@ static int clk_super_set_rate(struct clk_hw *hw, unsigned long rate,
>> return super->div_ops->set_rate(div_hw, rate, parent_rate);
>> }
>>
>> +static void clk_super_restore_context(struct clk_hw *hw)
>> +{
>> + struct tegra_clk_super_mux *super = to_clk_super_mux(hw);
>> + struct clk_hw *div_hw = &super->frac_div.hw;
>> + struct clk_hw *parent = clk_hw_get_parent(hw);
>> + int parent_id = clk_hw_get_parent_index(hw, parent);
>> +
>> + super->div_ops->restore_context(div_hw);
>> + clk_super_set_parent(hw, parent_id);
>> +}
>> +
>> const struct clk_ops tegra_clk_super_ops = {
>> .get_parent = clk_super_get_parent,
>> .set_parent = clk_super_set_parent,
>> .set_rate = clk_super_set_rate,
>> .round_rate = clk_super_round_rate,
>> .recalc_rate = clk_super_recalc_rate,
>> + .restore_context = clk_super_restore_context,
>> };
>>
>> struct clk *tegra_clk_register_super_mux(const char *name,
>>
^ permalink raw reply
* Re: [PATCH 2/3 v3] dt-bindings: mips: Add gardena vendor prefix and board description
From: Rob Herring @ 2019-08-09 17:07 UTC (permalink / raw)
To: Stefan Roese; +Cc: linux-mips, Paul Burton, devicetree
In-Reply-To: <20190809121918.25047-2-sr@denx.de>
On Fri, Aug 9, 2019 at 6:19 AM Stefan Roese <sr@denx.de> wrote:
>
> This patch adds the vendor prefix for gardena and a short description
> including the compatible string for the "GARDENA smart Gateway" based
> on the MT7688 SoC.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Paul Burton <paul.burton@mips.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> ---
> v3:
> - New patch
>
> .../devicetree/bindings/mips/ralink/gardena.txt | 9 +++++++++
> Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
> 2 files changed, 11 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mips/ralink/gardena.txt
Please add to ralink.txt rather than a new file. Ideally, that would
be converted to DT schema first, but given this is v3 already I won't
require that. A binding file per board vendor doesn't scale well and
will be a lot of duplication for schemas.
> diff --git a/Documentation/devicetree/bindings/mips/ralink/gardena.txt b/Documentation/devicetree/bindings/mips/ralink/gardena.txt
> new file mode 100644
> index 000000000000..4022fe61a8ee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mips/ralink/gardena.txt
> @@ -0,0 +1,9 @@
> +GARDENA smart Gateway (MT7688)
> +
> +This board is based on the MediaTek MT7688 and equipped with 128 MiB
> +of DDR and 8 MiB of flash (SPI NOR) and additional 128MiB SPI NAND
> +storage.
> +
> +------------------------------
> +Required root node properties:
> +- compatible = "gardena,smartGatewayMT7688";
You need an SoC compatible for MT7688 which isn't documented either.
Rob
^ permalink raw reply
* Re: [PATCH 3/4] regulator: qcom-rpmh: Fix pmic5_bob voltage count
From: Bjorn Andersson @ 2019-08-09 17:03 UTC (permalink / raw)
To: Vinod Koul
Cc: Mark Brown, linux-arm-msm, Andy Gross, Liam Girdwood, Rob Herring,
Mark Rutland, linux-kernel, devicetree
In-Reply-To: <20190809073616.1235-3-vkoul@kernel.org>
On Fri 09 Aug 00:36 PDT 2019, Vinod Koul wrote:
> pmic5_bob voltages count is 136 [0,135] so update it
>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> drivers/regulator/qcom-rpmh-regulator.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
> index 0ef2716da3bd..391ed844a251 100644
> --- a/drivers/regulator/qcom-rpmh-regulator.c
> +++ b/drivers/regulator/qcom-rpmh-regulator.c
> @@ -698,7 +698,7 @@ static const struct rpmh_vreg_hw_data pmic5_bob = {
> .regulator_type = VRM,
> .ops = &rpmh_regulator_vrm_bypass_ops,
> .voltage_range = REGULATOR_LINEAR_RANGE(300000, 0, 135, 32000),
> - .n_voltages = 135,
> + .n_voltages = 136,
> .pmic_mode_map = pmic_mode_map_pmic4_bob,
> .of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
> };
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH 2/4] regulator: qcom-rpmh: Sort the compatibles
From: Bjorn Andersson @ 2019-08-09 17:02 UTC (permalink / raw)
To: Vinod Koul
Cc: Mark Brown, linux-arm-msm, Andy Gross, Liam Girdwood, Rob Herring,
Mark Rutland, linux-kernel, devicetree
In-Reply-To: <20190809073616.1235-2-vkoul@kernel.org>
On Fri 09 Aug 00:36 PDT 2019, Vinod Koul wrote:
> It helps to keep sorted order for compatibles, so sort them
>
> Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> drivers/regulator/qcom-rpmh-regulator.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
> index 693ffec62f3e..0ef2716da3bd 100644
> --- a/drivers/regulator/qcom-rpmh-regulator.c
> +++ b/drivers/regulator/qcom-rpmh-regulator.c
> @@ -878,18 +878,14 @@ static int rpmh_regulator_probe(struct platform_device *pdev)
> }
>
> static const struct of_device_id rpmh_regulator_match_table[] = {
> - {
> - .compatible = "qcom,pm8998-rpmh-regulators",
> - .data = pm8998_vreg_data,
> - },
> - {
> - .compatible = "qcom,pmi8998-rpmh-regulators",
> - .data = pmi8998_vreg_data,
> - },
> {
> .compatible = "qcom,pm8005-rpmh-regulators",
> .data = pm8005_vreg_data,
> },
> + {
> + .compatible = "qcom,pm8009-rpmh-regulators",
> + .data = pm8009_vreg_data,
> + },
> {
> .compatible = "qcom,pm8150-rpmh-regulators",
> .data = pm8150_vreg_data,
> @@ -899,8 +895,12 @@ static const struct of_device_id rpmh_regulator_match_table[] = {
> .data = pm8150l_vreg_data,
> },
> {
> - .compatible = "qcom,pm8009-rpmh-regulators",
> - .data = pm8009_vreg_data,
> + .compatible = "qcom,pm8998-rpmh-regulators",
> + .data = pm8998_vreg_data,
> + },
> + {
> + .compatible = "qcom,pmi8998-rpmh-regulators",
> + .data = pmi8998_vreg_data,
> },
> {}
> };
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH 1/4] regulator: dt-bindings: Sort the compatibles and nodes
From: Bjorn Andersson @ 2019-08-09 17:01 UTC (permalink / raw)
To: Vinod Koul
Cc: Mark Brown, linux-arm-msm, Andy Gross, Liam Girdwood, Rob Herring,
Mark Rutland, linux-kernel, devicetree
In-Reply-To: <20190809073616.1235-1-vkoul@kernel.org>
On Fri 09 Aug 00:36 PDT 2019, Vinod Koul wrote:
> It helps to keep sorted order for compatibles and nodes, so sort them
>
> Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
> .../regulator/qcom,rpmh-regulator.txt | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt b/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt
> index 1a9cab50503a..bab9f71140b8 100644
> --- a/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt
> +++ b/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt
> @@ -22,12 +22,12 @@ RPMh resource.
>
> The names used for regulator nodes must match those supported by a given PMIC.
> Supported regulator node names:
> - PM8998: smps1 - smps13, ldo1 - ldo28, lvs1 - lvs2
> - PMI8998: bob
> PM8005: smps1 - smps4
> + PM8009: smps1 - smps2, ldo1 - ldo7
> PM8150: smps1 - smps10, ldo1 - ldo18
> PM8150L: smps1 - smps8, ldo1 - ldo11, bob, flash, rgb
> - PM8009: smps1 - smps2, ld01 - ldo7
> + PM8998: smps1 - smps13, ldo1 - ldo28, lvs1 - lvs2
> + PMI8998: bob
>
> ========================
> First Level Nodes - PMIC
> @@ -36,12 +36,13 @@ First Level Nodes - PMIC
> - compatible
> Usage: required
> Value type: <string>
> - Definition: Must be one of: "qcom,pm8998-rpmh-regulators",
> - "qcom,pmi8998-rpmh-regulators" or
> - "qcom,pm8005-rpmh-regulators" or
> - "qcom,pm8150-rpmh-regulators" or
> - "qcom,pm8150l-rpmh-regulators" or
> - "qcom,pm8009-rpmh-regulators".
> + Definition: Must be one of below:
> + "qcom,pm8005-rpmh-regulators"
> + "qcom,pm8009-rpmh-regulators"
> + "qcom,pm8150-rpmh-regulators"
> + "qcom,pm8150l-rpmh-regulators"
> + "qcom,pm8998-rpmh-regulators"
> + "qcom,pmi8998-rpmh-regulators"
Thanks for dropping the "or" as well.
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>
> - qcom,pmic-id
> Usage: required
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH v8 08/21] clk: tegra: periph: Add restore_context support
From: Sowjanya Komatineni @ 2019-08-09 16:55 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree, rjw, viresh.kumar, linux-pm
In-Reply-To: <0f8259d8-08f2-671c-331c-fe2d83518be0@gmail.com>
On 8/9/19 5:20 AM, Dmitry Osipenko wrote:
> 09.08.2019 14:55, Dmitry Osipenko пишет:
>> 09.08.2019 2:46, Sowjanya Komatineni пишет:
>>> This patch implements restore_context support for clk-periph and
>>> clk-sdmmc-mux clock operations to restore clock parent and rates
>>> on system resume.
>>>
>>> During system suspend, core power goes off and looses the context
>>> of the Tegra clock controller registers.
>>>
>>> So on system resume, clocks parent and rate are restored back to
>>> the context before suspend based on cached data.
>>>
>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>> ---
>>> drivers/clk/tegra/clk-periph.c | 18 ++++++++++++++++++
>>> drivers/clk/tegra/clk-sdmmc-mux.c | 12 ++++++++++++
>>> 2 files changed, 30 insertions(+)
>>>
>>> diff --git a/drivers/clk/tegra/clk-periph.c b/drivers/clk/tegra/clk-periph.c
>>> index 58437da25156..c9d28cbadccc 100644
>>> --- a/drivers/clk/tegra/clk-periph.c
>>> +++ b/drivers/clk/tegra/clk-periph.c
>>> @@ -3,6 +3,7 @@
>>> * Copyright (c) 2012, NVIDIA CORPORATION. All rights reserved.
>>> */
>>>
>>> +#include <linux/clk.h>
>>> #include <linux/clk-provider.h>
>>> #include <linux/export.h>
>>> #include <linux/slab.h>
>>> @@ -99,6 +100,20 @@ static void clk_periph_disable(struct clk_hw *hw)
>>> gate_ops->disable(gate_hw);
>>> }
>>>
>>> +static void clk_periph_restore_context(struct clk_hw *hw)
>>> +{
>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>> + const struct clk_ops *div_ops = periph->div_ops;
>>> + struct clk_hw *div_hw = &periph->divider.hw;
>>> + struct clk_hw *parent = clk_hw_get_parent(hw);
>>> + int parent_id = clk_hw_get_parent_index(hw, parent);
>>> +
>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_DIV))
>>> + div_ops->restore_context(div_hw);
>>> +
>>> + clk_periph_set_parent(hw, parent_id);
>>> +}
>>> +
>>> const struct clk_ops tegra_clk_periph_ops = {
>>> .get_parent = clk_periph_get_parent,
>>> .set_parent = clk_periph_set_parent,
>>> @@ -108,6 +123,7 @@ const struct clk_ops tegra_clk_periph_ops = {
>>> .is_enabled = clk_periph_is_enabled,
>>> .enable = clk_periph_enable,
>>> .disable = clk_periph_disable,
>>> + .restore_context = clk_periph_restore_context,
>>> };
>>>
>>> static const struct clk_ops tegra_clk_periph_nodiv_ops = {
>>> @@ -116,6 +132,7 @@ static const struct clk_ops tegra_clk_periph_nodiv_ops = {
>>> .is_enabled = clk_periph_is_enabled,
>>> .enable = clk_periph_enable,
>>> .disable = clk_periph_disable,
>>> + .restore_context = clk_periph_restore_context,
>>> };
>>>
>>> static const struct clk_ops tegra_clk_periph_no_gate_ops = {
>>> @@ -124,6 +141,7 @@ static const struct clk_ops tegra_clk_periph_no_gate_ops = {
>>> .recalc_rate = clk_periph_recalc_rate,
>>> .round_rate = clk_periph_round_rate,
>>> .set_rate = clk_periph_set_rate,
>>> + .restore_context = clk_periph_restore_context,
>>> };
>>>
>>> static struct clk *_tegra_clk_register_periph(const char *name,
>>> diff --git a/drivers/clk/tegra/clk-sdmmc-mux.c b/drivers/clk/tegra/clk-sdmmc-mux.c
>>> index a5cd3e31dbae..8db48966b100 100644
>>> --- a/drivers/clk/tegra/clk-sdmmc-mux.c
>>> +++ b/drivers/clk/tegra/clk-sdmmc-mux.c
>>> @@ -194,6 +194,17 @@ static void clk_sdmmc_mux_disable(struct clk_hw *hw)
>>> gate_ops->disable(gate_hw);
>>> }
>>>
>>> +static void clk_sdmmc_mux_restore_context(struct clk_hw *hw)
>>> +{
>>> + struct clk_hw *parent = clk_hw_get_parent(hw);
>>> + unsigned long parent_rate = clk_hw_get_rate(parent);
>>> + unsigned long rate = clk_hw_get_rate(hw);
>>> + int parent_id = clk_hw_get_parent_index(hw, parent);
>>> +
>>> + clk_sdmmc_mux_set_parent(hw, parent_id);
>>> + clk_sdmmc_mux_set_rate(hw, rate, parent_rate);
>> For the periph clocks you're restoring rate at first and then the
>> parent, while here it's the other way around. I'm wondering if there is
>> any difference in practice and thus whether rate's divider need to be
>> set to a some safe value before switching to a new parent that has a
>> higher clock rate than the old parent.
> Although, I guess that all peripheral clocks should be disabled at this
> point of resume. Correct?
by the time restore happens, peripheral clocks are enabled but held in
reset state.
For non-boot clocks, doing divider programming before parent source is
preferred.
For sdmmc clock, programming parent before divisor is allowed.
Also, clk_sdmmc_mux_set_rate gets parent MUX selection thru register
read so restoring parent prior to this will have right divisor based on
rate and parent.
>>> +}
>>> +
>>> static const struct clk_ops tegra_clk_sdmmc_mux_ops = {
>>> .get_parent = clk_sdmmc_mux_get_parent,
>>> .set_parent = clk_sdmmc_mux_set_parent,
>>> @@ -203,6 +214,7 @@ static const struct clk_ops tegra_clk_sdmmc_mux_ops = {
>>> .is_enabled = clk_sdmmc_mux_is_enabled,
>>> .enable = clk_sdmmc_mux_enable,
>>> .disable = clk_sdmmc_mux_disable,
>>> + .restore_context = clk_sdmmc_mux_restore_context,
>>> };
>>>
>>> struct clk *tegra_clk_register_sdmmc_mux_div(const char *name,
>>>
^ permalink raw reply
* Re: [PATCH 1/4] Input: ads7846 - convert to devm_ alloc functions
From: Dmitry Torokhov @ 2019-08-09 16:44 UTC (permalink / raw)
To: Marco Felsch; +Cc: robh+dt, kernel, linux-input, devicetree
In-Reply-To: <20190327133927.1340-2-m.felsch@pengutronix.de>
On Wed, Mar 27, 2019 at 02:39:24PM +0100, Marco Felsch wrote:
> Convert to devm function to drop the 'no-mem' error handling path and
> strip down the remove funciton a bit.
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
I am not fond of partial devm conversions, as this causes reordering in
of freeing resources in unwind path. In this particular case input
device will be unregistered only after majority of the driver structure
is torn down.
If we want to do devm conversion, we need to do a complete one.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/4] dt-bindings: watchdog: Add i.MX7ULP bindings
From: Guenter Roeck @ 2019-08-09 16:42 UTC (permalink / raw)
To: Anson Huang
Cc: wim, robh+dt, mark.rutland, shawnguo, s.hauer, kernel, festevam,
linux, otavio, leonard.crestez, schnitzeltony, u.kleine-koenig,
jan.tuerk, linux-watchdog, devicetree, linux-arm-kernel,
linux-kernel, Linux-imx
In-Reply-To: <1565334842-28161-1-git-send-email-Anson.Huang@nxp.com>
On Fri, Aug 09, 2019 at 03:13:59PM +0800, Anson Huang wrote:
> Add the watchdog bindings for Freescale i.MX7ULP.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> .../bindings/watchdog/fsl-imx7ulp-wdt.txt | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt
>
> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt b/Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt
> new file mode 100644
> index 0000000..d83fc5c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt
> @@ -0,0 +1,22 @@
> +* Freescale i.MX7ULP Watchdog Timer (WDT) Controller
> +
> +Required properties:
> +- compatible : Should be "fsl,imx7ulp-wdt"
> +- reg : Should contain WDT registers location and length
> +- interrupts : Should contain WDT interrupt
> +- clocks: Should contain a phandle pointing to the gated peripheral clock.
The driver as submitted does not include clock or interrupt handling.
Why are those properties listed as mandatory if they are not really
needed (nor used) ?
> +
> +Optional properties:
> +- timeout-sec : Contains the watchdog timeout in seconds
> +
> +Examples:
> +
> +wdog1: wdog@403d0000 {
> + compatible = "fsl,imx7ulp-wdt";
> + reg = <0x403d0000 0x10000>;
> + interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&pcc2 IMX7ULP_CLK_WDG1>;
> + assigned-clocks = <&pcc2 IMX7ULP_CLK_WDG1>;
> + assigned-clocks-parents = <&scg1 IMX7ULP_CLK_FIRC_BUS_CLK>;
> + timeout-sec = <40>;
> +};
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH 2/4] dt-bindings: input: ads7846: fix property description
From: Dmitry Torokhov @ 2019-08-09 16:42 UTC (permalink / raw)
To: Rob Herring; +Cc: Marco Felsch, robh+dt, kernel, linux-input, devicetree
In-Reply-To: <5ca06164.1c69fb81.39fb3.79f9@mx.google.com>
On Sun, Mar 31, 2019 at 01:42:42AM -0500, Rob Herring wrote:
> On Wed, 27 Mar 2019 14:39:25 +0100, Marco Felsch wrote:
> > The ti,y-max is used for the maximum value of the Y axis.
> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > Documentation/devicetree/bindings/input/touchscreen/ads7846.txt | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
>
> Reviewed-by: Rob Herring <robh@kernel.org>
>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 4/4] Input: ads7846 - add support for general touchscreen bindings
From: Dmitry Torokhov @ 2019-08-09 16:42 UTC (permalink / raw)
To: Marco Felsch; +Cc: robh+dt, kernel, linux-input, devicetree
In-Reply-To: <20190327133927.1340-5-m.felsch@pengutronix.de>
On Wed, Mar 27, 2019 at 02:39:27PM +0100, Marco Felsch wrote:
> A few vendor specific bindings are now covered by common bindings.
>
> Let the driver parse the common bindings to make use of common
> inverting and swapping mechnism. Aslo make use of
> touchscreen_report_pos() to ensure the correct inverting-swapping
> order.
>
> The vendor specific properties are used as default (backward
> compatibility) and gets overwritten by common bindings.
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Applied, thank you.
> ---
> drivers/input/touchscreen/ads7846.c | 38 +++++++++++++++++++++++------
> 1 file changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
> index 5a7a8425d619..2fe3b91f1db8 100644
> --- a/drivers/input/touchscreen/ads7846.c
> +++ b/drivers/input/touchscreen/ads7846.c
> @@ -23,6 +23,7 @@
> #include <linux/sched.h>
> #include <linux/delay.h>
> #include <linux/input.h>
> +#include <linux/input/touchscreen.h>
> #include <linux/interrupt.h>
> #include <linux/slab.h>
> #include <linux/pm.h>
> @@ -132,6 +133,8 @@ struct ads7846 {
>
> u16 penirq_recheck_delay_usecs;
>
> + struct touchscreen_properties core_prop;
> +
> struct mutex lock;
> bool stopped; /* P: lock */
> bool disabled; /* P: lock */
> @@ -826,17 +829,13 @@ static void ads7846_report_state(struct ads7846 *ts)
> if (Rt) {
> struct input_dev *input = ts->input;
>
> - if (ts->swap_xy)
> - swap(x, y);
> -
> if (!ts->pendown) {
> input_report_key(input, BTN_TOUCH, 1);
> ts->pendown = true;
> dev_vdbg(&ts->spi->dev, "DOWN\n");
> }
>
> - input_report_abs(input, ABS_X, x);
> - input_report_abs(input, ABS_Y, y);
> + touchscreen_report_pos(input, &ts->core_prop, x, y, false);
> input_report_abs(input, ABS_PRESSURE, ts->pressure_max - Rt);
>
> input_sync(input);
> @@ -1188,6 +1187,7 @@ static const struct ads7846_platform_data *ads7846_probe_dt(struct device *dev)
> struct ads7846_platform_data *pdata;
> struct device_node *node = dev->of_node;
> const struct of_device_id *match;
> + u32 value;
>
> if (!node) {
> dev_err(dev, "Device does not have associated DT data\n");
> @@ -1226,10 +1226,18 @@ static const struct ads7846_platform_data *ads7846_probe_dt(struct device *dev)
> of_property_read_u16(node, "ti,x-max", &pdata->x_max);
> of_property_read_u16(node, "ti,y-max", &pdata->y_max);
>
> + /*
> + * touchscreen-max-pressure gets parsed during
> + * touchscreen_parse_properties()
> + */
> of_property_read_u16(node, "ti,pressure-min", &pdata->pressure_min);
> + if (!of_property_read_u32(node, "touchscreen-min-pressure", &value))
> + pdata->pressure_min = (u16) value;
> of_property_read_u16(node, "ti,pressure-max", &pdata->pressure_max);
>
> of_property_read_u16(node, "ti,debounce-max", &pdata->debounce_max);
> + if (!of_property_read_u32(node, "touchscreen-average-samples", &value))
> + pdata->debounce_max = (u16) value;
> of_property_read_u16(node, "ti,debounce-tol", &pdata->debounce_tol);
> of_property_read_u16(node, "ti,debounce-rep", &pdata->debounce_rep);
>
> @@ -1314,10 +1322,7 @@ static int ads7846_probe(struct spi_device *spi)
> ts->model = pdata->model ? : 7846;
> ts->vref_delay_usecs = pdata->vref_delay_usecs ? : 100;
> ts->x_plate_ohms = pdata->x_plate_ohms ? : 400;
> - ts->pressure_max = pdata->pressure_max ? : ~0;
> -
> ts->vref_mv = pdata->vref_mv;
> - ts->swap_xy = pdata->swap_xy;
>
> if (pdata->filter != NULL) {
> if (pdata->filter_init != NULL) {
> @@ -1368,6 +1373,23 @@ static int ads7846_probe(struct spi_device *spi)
> input_set_abs_params(input_dev, ABS_PRESSURE,
> pdata->pressure_min, pdata->pressure_max, 0, 0);
>
> + /*
> + * Parse common framework properties. Must be done here to ensure the
> + * correct behaviour in case of using the legacy vendor bindings. The
> + * general binding value overrides the vendor specific one.
> + */
> + touchscreen_parse_properties(ts->input, false, &ts->core_prop);
> + ts->pressure_max = input_abs_get_max(input_dev, ABS_PRESSURE) ? : ~0;
> +
> + /*
> + * Check if legacy ti,swap-xy binding is used instead of
> + * touchscreen-swapped-x-y
> + */
> + if (!ts->core_prop.swap_x_y && pdata->swap_xy) {
> + swap(input_dev->absinfo[ABS_X], input_dev->absinfo[ABS_Y]);
> + ts->core_prop.swap_x_y = true;
> + }
> +
> ads7846_setup_spi_msg(ts, pdata);
>
> ts->reg = regulator_get(&spi->dev, "vcc");
> --
> 2.20.1
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH v8 11/21] clk: tegra: clk-dfll: Add suspend and resume support
From: Sowjanya Komatineni @ 2019-08-09 16:39 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree, rjw, viresh.kumar, linux-pm
In-Reply-To: <eb4fdab8-aba3-7f0c-a391-d751674fd03e@gmail.com>
On 8/9/19 5:23 AM, Dmitry Osipenko wrote:
> 09.08.2019 2:46, Sowjanya Komatineni пишет:
>> This patch implements DFLL suspend and resume operation.
>>
>> During system suspend entry, CPU clock will switch CPU to safe
>> clock source of PLLP and disables DFLL clock output.
>>
>> DFLL driver suspend confirms DFLL disable state and errors out on
>> being active.
>>
>> DFLL is re-initialized during the DFLL driver resume as it goes
>> through complete reset during suspend entry.
>>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
>> drivers/clk/tegra/clk-dfll.c | 56 ++++++++++++++++++++++++++++++
>> drivers/clk/tegra/clk-dfll.h | 2 ++
>> drivers/clk/tegra/clk-tegra124-dfll-fcpu.c | 1 +
>> 3 files changed, 59 insertions(+)
>>
>> diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
>> index f8688c2ddf1a..eb298a5d7be9 100644
>> --- a/drivers/clk/tegra/clk-dfll.c
>> +++ b/drivers/clk/tegra/clk-dfll.c
>> @@ -1487,6 +1487,7 @@ static int dfll_init(struct tegra_dfll *td)
>> td->last_unrounded_rate = 0;
>>
>> pm_runtime_enable(td->dev);
>> + pm_runtime_irq_safe(td->dev);
>> pm_runtime_get_sync(td->dev);
>>
>> dfll_set_mode(td, DFLL_DISABLED);
>> @@ -1513,6 +1514,61 @@ static int dfll_init(struct tegra_dfll *td)
>> return ret;
>> }
>>
>> +/**
>> + * tegra_dfll_suspend - check DFLL is disabled
>> + * @dev: DFLL device *
>> + *
>> + * DFLL clock should be disabled by the CPUFreq driver. So, make
>> + * sure it is disabled and disable all clocks needed by the DFLL.
>> + */
>> +int tegra_dfll_suspend(struct device *dev)
>> +{
>> + struct tegra_dfll *td = dev_get_drvdata(dev);
>> +
>> + if (dfll_is_running(td)) {
>> + dev_err(td->dev, "dfll is enabled while shouldn't be\n");
>> + return -EBUSY;
>> + }
>> +
>> + reset_control_assert(td->dvco_rst);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(tegra_dfll_suspend);
>> +
>> +/**
>> + * tegra_dfll_resume - reinitialize DFLL on resume
>> + * @dev: DFLL instance
>> + *
>> + * DFLL is disabled and reset during suspend and resume.
>> + * So, reinitialize the DFLL IP block back for use.
>> + * DFLL clock is enabled later in closed loop mode by CPUFreq
>> + * driver before switching its clock source to DFLL output.
>> + */
>> +int tegra_dfll_resume(struct device *dev)
>> +{
>> + struct tegra_dfll *td = dev_get_drvdata(dev);
>> +
>> + reset_control_deassert(td->dvco_rst);
> This doesn't look right because I assume that DFLL resetting is
> synchronous and thus clk should be enabled in order for reset to
> propagate inside hardware.
>
>> + pm_runtime_get_sync(td->dev);
> Hence it will be better to remove the above reset_control_deassert() and
> add here:
>
> reset_control_reset(td->dvco_rst);
By the time dfll resume happens, dfll controller clock will already be
enabled.
so doing reset de-assert before pm_runtime seems ok.
>> + dfll_set_mode(td, DFLL_DISABLED);
>> + dfll_set_default_params(td);
>> +
>> + if (td->soc->init_clock_trimmers)
>> + td->soc->init_clock_trimmers();
>> +
>> + dfll_set_open_loop_config(td);
>> +
>> + dfll_init_out_if(td);
>> +
>> + pm_runtime_put_sync(td->dev);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(tegra_dfll_resume);
>> +
>> /*
>> * DT data fetch
>> */
>> diff --git a/drivers/clk/tegra/clk-dfll.h b/drivers/clk/tegra/clk-dfll.h
>> index 1b14ebe7268b..fb209eb5f365 100644
>> --- a/drivers/clk/tegra/clk-dfll.h
>> +++ b/drivers/clk/tegra/clk-dfll.h
>> @@ -42,5 +42,7 @@ int tegra_dfll_register(struct platform_device *pdev,
>> struct tegra_dfll_soc_data *tegra_dfll_unregister(struct platform_device *pdev);
>> int tegra_dfll_runtime_suspend(struct device *dev);
>> int tegra_dfll_runtime_resume(struct device *dev);
>> +int tegra_dfll_suspend(struct device *dev);
>> +int tegra_dfll_resume(struct device *dev);
>>
>> #endif /* __DRIVERS_CLK_TEGRA_CLK_DFLL_H */
>> diff --git a/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c b/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
>> index e84b6d52cbbd..2ac2679d696d 100644
>> --- a/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
>> +++ b/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
>> @@ -631,6 +631,7 @@ static int tegra124_dfll_fcpu_remove(struct platform_device *pdev)
>> static const struct dev_pm_ops tegra124_dfll_pm_ops = {
>> SET_RUNTIME_PM_OPS(tegra_dfll_runtime_suspend,
>> tegra_dfll_runtime_resume, NULL)
>> + SET_SYSTEM_SLEEP_PM_OPS(tegra_dfll_suspend, tegra_dfll_resume)
>> };
>>
>> static struct platform_driver tegra124_dfll_fcpu_driver = {
>>
^ permalink raw reply
* Re: [PATCH] Add optional chip erase functionality to AT25 EEPROM driver.
From: Joe Burmeister @ 2019-08-09 16:28 UTC (permalink / raw)
To: Mark Rutland
Cc: Rob Herring, Arnd Bergmann, Greg Kroah-Hartman,
Srinivas Kandagatla, YueHaibing, Bartosz Golaszewski, devicetree,
linux-kernel
In-Reply-To: <20190809135440.GI48423@lakrids.cambridge.arm.com>
Hi Mark,
On 09/08/2019 14:54, Mark Rutland wrote:
> On Fri, Aug 09, 2019 at 01:53:55PM +0100, Joe Burmeister wrote:
>> Many, though not all, AT25s have an instruction for chip erase.
>> If there is one in the datasheet, it can be added to device tree.
>> Erase can then be done in userspace via the sysfs API with a new
>> "erase" device attribute. This matches the eeprom_93xx46 driver's
>> "erase".
>>
>> Signed-off-by: Joe Burmeister <joe.burmeister@devtank.co.uk>
>> ---
>> .../devicetree/bindings/eeprom/at25.txt | 2 +
>> drivers/misc/eeprom/at25.c | 83 ++++++++++++++++++-
>> 2 files changed, 82 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/eeprom/at25.txt b/Documentation/devicetree/bindings/eeprom/at25.txt
>> index b3bde97dc199..c65d11e14c7a 100644
>> --- a/Documentation/devicetree/bindings/eeprom/at25.txt
>> +++ b/Documentation/devicetree/bindings/eeprom/at25.txt
>> @@ -19,6 +19,7 @@ Optional properties:
>> - spi-cpha : SPI shifted clock phase, as per spi-bus bindings.
>> - spi-cpol : SPI inverse clock polarity, as per spi-bus bindings.
>> - read-only : this parameter-less property disables writes to the eeprom
>> +- chip_erase_instruction : Chip erase instruction for this AT25, often 0xc7 or 0x62.
> This should be using '-' rather than '_', as per general DT conventions
> and as with the existing properties.
>
>> Obsolete legacy properties can be used in place of "size", "pagesize",
>> "address-width", and "read-only":
>> @@ -39,4 +40,5 @@ Example:
>> pagesize = <64>;
>> size = <32768>;
>> address-width = <16>;
>> + chip_erase_instruction = <0x62>;
> [...]
>
>> + /* Optional chip erase instruction */
>> + device_property_read_u8(&spi->dev, "chip_erase_instruction", &at25->erase_instr);
> This will not behave as you expect, since you didn't mark the property as
> 8-bits.
Rats, I forgot to update the documentation. In my device tree I have
already found the /bit 8/ bit.
> Read this as a u32 into the existing val temporary variable, as is done
> for pagesize. You can add a warnign if it's out-of-range.
32bit would certainly read better in the device tree. I'll do that.
> Thanks,
> Mark.
Cheers,
Joe
^ permalink raw reply
* Re: [PATCH 3/3] drm/bridge: Add NWL MIPI DSI host controller support
From: Guido Günther @ 2019-08-09 16:25 UTC (permalink / raw)
To: Laurent Pinchart
Cc: David Airlie, Daniel Vetter, Rob Herring, Mark Rutland, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP Linux Team, Andrzej Hajda, Neil Armstrong, Jonas Karlman,
Jernej Skrabec, Lee Jones, dri-devel, devicetree,
linux-arm-kernel, linux-kernel, Robert Chiras
In-Reply-To: <20190727024700.GD4902@pendragon.ideasonboard.com>
Hi Laurent,
thanks for the review! Most of it seemed clear how to fix for the rest
i've put some questions below:
On Sat, Jul 27, 2019 at 05:47:00AM +0300, Laurent Pinchart wrote:
> Hello Guido,
>
> Thank you for the patch.
>
> On Wed, Jul 24, 2019 at 05:52:26PM +0200, Guido Günther wrote:
> > This adds initial support for the NWL MIPI DSI Host controller found on
> > i.MX8 SoCs.
> >
> > It adds support for the i.MX8MQ but the same IP can be found on
> > e.g. the i.MX8QXP.
> >
> > It has been tested on the Librem 5 devkit using mxsfb.
> >
> > Signed-off-by: Guido Günther <agx@sigxcpu.org>
> > Co-developed-by: Robert Chiras <robert.chiras@nxp.com>
> > ---
> > drivers/gpu/drm/bridge/Kconfig | 2 +
> > drivers/gpu/drm/bridge/Makefile | 1 +
> > drivers/gpu/drm/bridge/imx-nwl/Kconfig | 15 +
> > drivers/gpu/drm/bridge/imx-nwl/Makefile | 2 +
> > drivers/gpu/drm/bridge/imx-nwl/nwl-drv.c | 529 ++++++++++++++++
> > drivers/gpu/drm/bridge/imx-nwl/nwl-drv.h | 72 +++
> > drivers/gpu/drm/bridge/imx-nwl/nwl-dsi.c | 745 +++++++++++++++++++++++
> > drivers/gpu/drm/bridge/imx-nwl/nwl-dsi.h | 111 ++++
> > 8 files changed, 1477 insertions(+)
> > create mode 100644 drivers/gpu/drm/bridge/imx-nwl/Kconfig
> > create mode 100644 drivers/gpu/drm/bridge/imx-nwl/Makefile
> > create mode 100644 drivers/gpu/drm/bridge/imx-nwl/nwl-drv.c
> > create mode 100644 drivers/gpu/drm/bridge/imx-nwl/nwl-drv.h
> > create mode 100644 drivers/gpu/drm/bridge/imx-nwl/nwl-dsi.c
> > create mode 100644 drivers/gpu/drm/bridge/imx-nwl/nwl-dsi.h
> >
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index a6eec908c43e..38c3145a7e57 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -152,6 +152,8 @@ source "drivers/gpu/drm/bridge/analogix/Kconfig"
> >
> > source "drivers/gpu/drm/bridge/adv7511/Kconfig"
> >
> > +source "drivers/gpu/drm/bridge/imx-nwl/Kconfig"
> > +
>
> As this doesn't seem to be an i.MX-specific IP, I wouldn't use the name
> imx in file names or in the code, at least in the parts that are not
> NXP-specific.
O.k. Since i've not seen other SoCs using this ip core I wasn't sure
what would be sharable but we'll figure that out. Renamed to nwl-dsi/
> > source "drivers/gpu/drm/bridge/synopsys/Kconfig"
> >
> > endmenu
> > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> > index 4934fcf5a6f8..904a9eb3a20a 100644
> > --- a/drivers/gpu/drm/bridge/Makefile
> > +++ b/drivers/gpu/drm/bridge/Makefile
> > @@ -16,4 +16,5 @@ obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
> > obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
> > obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o
> > obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
> > +obj-y += imx-nwl/
> > obj-y += synopsys/
> > diff --git a/drivers/gpu/drm/bridge/imx-nwl/Kconfig b/drivers/gpu/drm/bridge/imx-nwl/Kconfig
> > new file mode 100644
> > index 000000000000..822dba1b380a
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/imx-nwl/Kconfig
> > @@ -0,0 +1,15 @@
> > +config DRM_IMX_NWL_DSI
> > + tristate "Support for Northwest Logic MIPI DSI Host controller"
> > + depends on DRM && (ARCH_MXC || ARCH_MULTIPLATFORM || COMPILE_TEST)
> > + depends on COMMON_CLK
> > + depends on OF && HAS_IOMEM
> > + select DRM_KMS_HELPER
> > + select DRM_MIPI_DSI
> > + select DRM_PANEL_BRIDGE
> > + select GENERIC_PHY_MIPI_DPHY
> > + select MFD_SYSCON
> > + select REGMAP_MMIO
> > + help
> > + This enables the Northwest Logic MIPI DSI Host controller as
> > + found on NXP's i.MX8 Processors.
> > +
> > diff --git a/drivers/gpu/drm/bridge/imx-nwl/Makefile b/drivers/gpu/drm/bridge/imx-nwl/Makefile
> > new file mode 100644
> > index 000000000000..9fa63483da5b
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/imx-nwl/Makefile
> > @@ -0,0 +1,2 @@
> > +imx-nwl-objs := nwl-drv.o nwl-dsi.o
> > +obj-$(CONFIG_DRM_IMX_NWL_DSI) += imx-nwl.o
> > diff --git a/drivers/gpu/drm/bridge/imx-nwl/nwl-drv.c b/drivers/gpu/drm/bridge/imx-nwl/nwl-drv.c
> > new file mode 100644
> > index 000000000000..451f8f067c6f
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/imx-nwl/nwl-drv.c
> > @@ -0,0 +1,529 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * i.MX8 NWL MIPI DSI host driver
> > + *
> > + * Copyright (C) 2017 NXP
> > + * Copyright (C) 2019 Purism SPC
> > + */
> > +
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_of.h>
> > +#include <drm/drm_panel.h>
> > +#include <drm/drm_print.h>
> > +#include <drm/drm_probe_helper.h>
> > +#include <linux/clk-provider.h>
>
> This doesn't seem to be needed.
Dropped.
>
> > +#include <linux/clk.h>
> > +#include <linux/component.h>
>
> Same here.
Dropped (it was a component driver before).
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/irq.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/mfd/syscon/imx8mq-iomuxc-gpr.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/phy/phy.h>
> > +#include <linux/regmap.h>
> > +#include <linux/sys_soc.h>
> > +#include <video/videomode.h>
> > +
> > +#include "nwl-drv.h"
> > +#include "nwl-dsi.h"
> > +
> > +#define DRV_NAME "imx-nwl-dsi"
> > +
> > +/* 8MQ SRC specific registers */
> > +#define SRC_MIPIPHY_RCR 0x28
> > +#define RESET_BYTE_N BIT(1)
> > +#define RESET_N BIT(2)
> > +#define DPI_RESET_N BIT(3)
> > +#define ESC_RESET_N BIT(4)
> > +#define PCLK_RESET_N BIT(5)
> > +
> > +/* Possible clocks */
> > +#define CLK_PIXEL "pixel"
> > +#define CLK_CORE "core"
> > +#define CLK_BYPASS "bypass"
> > +
> > +enum imx_ext_regs {
> > + IMX_REG_CSR = BIT(1),
> > + IMX_REG_SRC = BIT(2),
> > + IMX_REG_GPR = BIT(3),
> > +};
> > +
> > +static const struct regmap_config nwl_dsi_regmap_config = {
> > + .reg_bits = 16,
> > + .val_bits = 32,
> > + .reg_stride = 4,
> > + .max_register = IRQ_MASK2,
> > + .name = DRV_NAME,
> > +};
> > +
> > +struct imx_nwl_platform_data {
> > + int (*poweron)(struct imx_nwl_dsi *dsi);
> > + int (*poweroff)(struct imx_nwl_dsi *dsi);
> > + u32 ext_regs; /* required external registers */
> > + struct imx_nwl_clk_config clk_config[NWL_MAX_PLATFORM_CLOCKS];
> > +};
> > +
> > +static inline struct imx_nwl_dsi *bridge_to_dsi(struct drm_bridge *bridge)
> > +{
> > + return container_of(bridge, struct imx_nwl_dsi, bridge);
> > +}
> > +
> > +static void imx_nwl_dsi_set_clocks(struct imx_nwl_dsi *dsi, bool enable)
> > +{
> > + struct device *dev = dsi->dev;
> > + const char *id;
> > + struct clk *clk;
> > + unsigned long new_rate, cur_rate;
> > + bool enabled;
> > + size_t i;
> > + int ret;
> > +
> > + DRM_DEV_DEBUG_DRIVER(dev, "%sabling platform clocks",
> > + enable ? "en" : "dis");
> > + for (i = 0; i < ARRAY_SIZE(dsi->pdata->clk_config); i++) {
> > + if (!dsi->clk_config[i].present)
> > + continue;
> > + id = dsi->clk_config[i].id;
> > + clk = dsi->clk_config[i].clk;
> > + new_rate = dsi->clk_config[i].rate;
> > + cur_rate = clk_get_rate(clk);
> > + enabled = dsi->clk_config[i].enabled;
> > +
> > + /* BYPASS clk must have the same rate as PHY_REF clk */
> > + if (!strcmp(id, CLK_BYPASS))
> > + new_rate = clk_get_rate(dsi->phy_ref_clk);
> > +
> > + if (enable) {
> > + if (enabled && new_rate != cur_rate)
> > + clk_disable_unprepare(clk);
> > + else if (enabled && new_rate == cur_rate)
> > + continue;
> > + if (new_rate > 0)
> > + clk_set_rate(clk, new_rate);
> > + ret = clk_prepare_enable(clk);
> > + if (ret < 0) {
> > + DRM_DEV_ERROR(dev, "Failed to enable clock %s",
> > + id);
> > + }
> > + dsi->clk_config[i].enabled = true;
> > + cur_rate = clk_get_rate(clk);
> > + DRM_DEV_DEBUG_DRIVER(
> > + dev, "Enabled %s clk (rate: req=%lu act=%lu)\n",
> > + id, new_rate, cur_rate);
> > + } else if (enabled) {
> > + clk_disable_unprepare(clk);
> > + dsi->clk_config[i].enabled = false;
> > + DRM_DEV_DEBUG_DRIVER(dev, "Disabled %s clk\n", id);
> > + }
> > + }
> > +}
> > +
> > +static void imx_nwl_dsi_enable(struct imx_nwl_dsi *dsi)
> > +{
> > + struct device *dev = dsi->dev;
> > + int ret;
> > +
> > + imx_nwl_dsi_set_clocks(dsi, true);
> > +
> > + ret = dsi->pdata->poweron(dsi);
> > + if (ret < 0)
> > + DRM_DEV_ERROR(dev, "Failed to power on DSI (%d)\n", ret);
> > +}
> > +
> > +static void imx_nwl_dsi_disable(struct imx_nwl_dsi *dsi)
> > +{
> > + struct device *dev = dsi->dev;
> > +
> > + if (dsi->dpms_mode != DRM_MODE_DPMS_ON)
> > + return;
> > +
>
> The DRM core should guarantee that the bridge won't be disabled twice,
> so I don't think you need this check. Similarly I think the enabled flag
> in the imx_nwl_clk_config structure can be removed.
Dropped and i also simplified the imx_nwl_clk_config - rates are supplied
via DT here anyway.
>
> > + DRM_DEV_DEBUG_DRIVER(dev, "Disabling encoder");
>
> Is this really needed ?
Dropped.
> > + dsi->pdata->poweroff(dsi);
> > + imx_nwl_dsi_set_clocks(dsi, false);
> > +}
> > +
> > +static void imx_nwl_select_input_source(struct imx_nwl_dsi *dsi)
> > +{
> > + struct device_node *remote;
> > + u32 mux_val = IMX8MQ_GPR13_MIPI_MUX_SEL;
> > +
> > + remote = of_graph_get_remote_node(dsi->dev->of_node, 0, 0);
> > + if (strcmp(remote->name, "lcdif") == 0)
> > + mux_val = 0;
> > +
Getting it from dt spares us carrying around more platform specific
state. Should i change it anyway?
>
> Can't you check the remote node at probe time instead of every time the
> bridge gets enabled, and program the IO mux accordingly there ?
>
> This code is i.MX-specific, so it should be isolated in an operation in
> struct imx_nwl_platform_data.
Done.
>
> > + DRM_DEV_INFO(dsi->dev, "Using %s as input source\n",
> > + (mux_val) ? "DCSS" : "LCDIF");
> > + regmap_update_bits(dsi->mux_sel, IOMUXC_GPR13,
> > + IMX8MQ_GPR13_MIPI_MUX_SEL, mux_val);
> > + of_node_put(remote);
> > +}
> > +
> > +static void imx_nwl_dsi_bridge_disable(struct drm_bridge *bridge)
> > +{
> > + struct imx_nwl_dsi *dsi = bridge_to_dsi(bridge);
> > +
> > + if (dsi->dpms_mode != DRM_MODE_DPMS_ON)
> > + return;
> > +
> > + nwl_dsi_disable(dsi);
> > + imx_nwl_dsi_disable(dsi);
> > + pm_runtime_put_sync(dsi->dev);
>
> Do you need a put_sync, wouldn't a put do ?
Switched to put() only.
>
> > + dsi->dpms_mode = DRM_MODE_DPMS_OFF;
> > +}
> > +
> > +static bool
> > +imx_nwl_dsi_bridge_mode_fixup(struct drm_bridge *bridge,
> > + const struct drm_display_mode *mode,
> > + struct drm_display_mode *adjusted_mode)
> > +{
> > + struct imx_nwl_dsi *dsi = bridge_to_dsi(bridge);
> > + struct device *dev = dsi->dev;
> > + union phy_configure_opts new_cfg;
> > + unsigned long phy_ref_rate;
> > + int ret;
> > +
> > + ret = nwl_dsi_get_dphy_params(dsi, adjusted_mode, &new_cfg);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /*
> > + * If hs clock is unchanged, we're all good - all parameters are
> > + * derived from it atm.
> > + */
> > + if (new_cfg.mipi_dphy.hs_clk_rate == dsi->phy_cfg.mipi_dphy.hs_clk_rate)
> > + return true;
> > +
> > + phy_ref_rate = clk_get_rate(dsi->phy_ref_clk);
> > + DRM_DEV_DEBUG_DRIVER(dev, "PHY at ref rate: %lu\n", phy_ref_rate);
> > + if (ret < 0) {
>
> This can't happen. Or are you missing a function call before the check
> ?
The code used that a long time ago, fixed that path.
>
> > + DRM_DEV_ERROR(dsi->dev,
> > + "Cannot setup PHY for mode: %ux%u @%d Hz\n",
> > + adjusted_mode->hdisplay, adjusted_mode->vdisplay,
> > + adjusted_mode->clock);
> > + DRM_DEV_ERROR(dsi->dev, "PHY ref clk: %lu, bit clk: %lu\n",
> > + phy_ref_rate, new_cfg.mipi_dphy.hs_clk_rate);
> > + } else {
> > + /* Save the new desired phy config */
> > + memcpy(&dsi->phy_cfg, &new_cfg, sizeof(new_cfg));
>
> The mode_fixup operation shall not change the device state, it can be
> called multiple times when trying modes.
Moved the parts to 'mode_set' instead.
>
> > + }
> > +
> > + /* LCDIF + NWL needs active high sync */
> > + adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
> > + adjusted_mode->flags &= ~(DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
> > +
> > + drm_display_mode_to_videomode(adjusted_mode, &dsi->vm);
> > + drm_mode_debug_printmodeline(adjusted_mode);
> > +
> > + return ret == 0;
>
> return 0;
mode_fixup wants a bool but that code is gone anyways.
>
> > +}
> > +
> > +static enum drm_mode_status
> > +imx_nwl_dsi_bridge_mode_valid(struct drm_bridge *bridge,
> > + const struct drm_display_mode *mode)
> > +{
> > + struct imx_nwl_dsi *dsi = bridge_to_dsi(bridge);
> > + int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> > +
> > + if (bpp < 0) {
> > + DRM_DEV_ERROR(dsi->dev, "Invalid pixel format: %d\n",
> > + dsi->format);
> > + return MODE_BAD;
> > + }
>
> The format isn't part of the mode, so this doesn't belong here. You
> should here instead check that the mode clock and other timing data
> (especially the visible resolution) are within the range supported by
> the device.
O.k. I've added a clock check derived from the spec instead.
> > +
> > + return MODE_OK;
> > +}
> > +
> > +static void imx_nwl_dsi_bridge_pre_enable(struct drm_bridge *bridge)
> > +{
> > + struct imx_nwl_dsi *dsi = bridge_to_dsi(bridge);
> > +
> > + if (dsi->dpms_mode == DRM_MODE_DPMS_ON)
> > + return;
> > +
> > + imx_nwl_select_input_source(dsi);
> > + pm_runtime_get_sync(dsi->dev);
> > + imx_nwl_dsi_enable(dsi);
> > + nwl_dsi_enable(dsi);
> > + dsi->dpms_mode = DRM_MODE_DPMS_ON;
> > +}
> > +
> > +static int imx_nwl_dsi_bridge_attach(struct drm_bridge *bridge)
> > +{
> > + struct imx_nwl_dsi *dsi = bridge->driver_private;
> > + struct drm_encoder *encoder = bridge->encoder;
> > +
> > + if (!encoder) {
> > + DRM_DEV_ERROR(dsi->dev, "Parent encoder object not found\n");
> > + return -ENODEV;
> > + }
>
> Can't this happen ?
Dropped.
>
> > +
> > + /* Set the encoder type as caller does not know it */
> > + bridge->encoder->encoder_type = DRM_MODE_ENCODER_DSI;
>
> The encoder type is quite meaningless and userspace should not depend on
> it, so I wouldn't set it here, especially that the encoder may not
> expect the bridge to override its type.
Dropped.
>
> > +
> > + /* Attach the panel-bridge to the dsi bridge */
> > + return drm_bridge_attach(bridge->encoder, dsi->panel_bridge, bridge);
> > +}
> > +
> > +static void imx_nwl_dsi_bridge_detach(struct drm_bridge *bridge)
> > +{
> > + struct imx_nwl_dsi *dsi = bridge->driver_private;
> > +
> > + drm_of_panel_bridge_remove(dsi->dev->of_node, 1, 0);
>
> This is already done in nwl_dsi_host_detach().
Dropped the whole detach.
>
> > +}
> > +
> > +/* see dw-mipi-dsi.c */
>
> What for ? :-)
Dropped.
>
> > +static const struct drm_bridge_funcs imx_nwl_dsi_bridge_funcs = {
> > + .pre_enable = imx_nwl_dsi_bridge_pre_enable,
> > + .disable = imx_nwl_dsi_bridge_disable,
> > + .mode_fixup = imx_nwl_dsi_bridge_mode_fixup,
> > + .mode_valid = imx_nwl_dsi_bridge_mode_valid,
> > + .attach = imx_nwl_dsi_bridge_attach,
> > + .detach = imx_nwl_dsi_bridge_detach,
> > +};
> > +
> > +static int imx_nwl_dsi_parse_dt(struct imx_nwl_dsi *dsi)
> > +{
> > + struct device_node *np = dsi->dev->of_node;
> > + struct platform_device *pdev = to_platform_device(dsi->dev);
> > + struct resource *res;
> > + struct clk *clk;
> > + const char *clk_id;
> > + void __iomem *base;
> > + int i, ret;
> > +
> > + dsi->phy = devm_phy_get(dsi->dev, "dphy");
> > + if (IS_ERR(dsi->phy)) {
> > + ret = PTR_ERR(dsi->phy);
> > + dev_err(dsi->dev, "Could not get PHY (%d)\n", ret);
> > + return ret;
> > + }
> > +
> > + /* Platform dependent clocks */
> > + memcpy(dsi->clk_config, dsi->pdata->clk_config,
> > + sizeof(dsi->pdata->clk_config));
> > +
> > + for (i = 0; i < ARRAY_SIZE(dsi->pdata->clk_config); i++) {
> > + if (!dsi->clk_config[i].present)
> > + continue;
> > +
> > + clk_id = dsi->clk_config[i].id;
> > + clk = devm_clk_get(dsi->dev, clk_id);
> > + if (IS_ERR(clk)) {
> > + ret = PTR_ERR(clk);
> > + dev_err(dsi->dev, "Failed to get %s clock (%d)\n",
> > + clk_id, ret);
> > + return ret;
> > + }
> > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "Setup clk %s (rate: %lu)\n",
> > + clk_id, clk_get_rate(clk));
> > + dsi->clk_config[i].clk = clk;
> > + }
> > +
> > + /* DSI clocks */
> > + clk = devm_clk_get(dsi->dev, "phy_ref");
> > + if (IS_ERR(clk)) {
> > + ret = PTR_ERR(clk);
> > + dev_err(dsi->dev, "Failed to get phy_ref clock: %d\n", ret);
> > + return ret;
> > + }
> > + dsi->phy_ref_clk = clk;
> > +
> > + clk = devm_clk_get(dsi->dev, "rx_esc");
> > + if (IS_ERR(clk)) {
> > + ret = PTR_ERR(clk);
> > + dev_err(dsi->dev, "Failed to get rx_esc clock: %d\n", ret);
> > + return ret;
> > + }
> > + dsi->rx_esc_clk = clk;
> > +
> > + clk = devm_clk_get(dsi->dev, "tx_esc");
> > + if (IS_ERR(clk)) {
> > + ret = PTR_ERR(clk);
> > + dev_err(dsi->dev, "Failed to get tx_esc clock: %d\n", ret);
> > + return ret;
> > + }
> > + dsi->tx_esc_clk = clk;
> > +
> > + dsi->csr = syscon_regmap_lookup_by_phandle(np, "csr");
> > + if (IS_ERR(dsi->csr) && dsi->pdata->ext_regs & IMX_REG_CSR) {
> > + ret = PTR_ERR(dsi->csr);
> > + DRM_DEV_ERROR(dsi->dev, "Failed to get CSR regmap: %d\n",
> > + ret);
> > + return ret;
> > + }
>
> This doesn't seem to be used anywhere.
That's true. i had it in for the imx8q*, dropped that (and some other
parts) until support for that soc is added.
>
> > + dsi->reset = syscon_regmap_lookup_by_phandle(np, "src");
> > + if (IS_ERR(dsi->reset) && (dsi->pdata->ext_regs & IMX_REG_SRC)) {
> > + ret = PTR_ERR(dsi->reset);
> > + DRM_DEV_ERROR(dsi->dev, "Failed to get SRC regmap: %d\n",
> > + ret);
> > + return ret;
> > + }
>
> Couldn't you model a reset controller in that syscon, and use the reset
> controller API here ? It would allow moving the i.MX-specific power on
> and off functions from this driver, making it more generic.
In fact the reset controller is already there, wired it up accordingly.
>
> > + dsi->mux_sel = syscon_regmap_lookup_by_phandle(np, "mux-sel");
> > + if (IS_ERR(dsi->mux_sel) && (dsi->pdata->ext_regs & IMX_REG_GPR)) {
> > + ret = PTR_ERR(dsi->mux_sel);
> > + DRM_DEV_ERROR(dsi->dev, "Failed to get GPR regmap: %d\n",
> > + ret);
> > + return ret;
> > + }
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + base = devm_ioremap_resource(dsi->dev, res);
>
> You can replace those two calls with devm_platform_ioremap_resource().
Done.
>
> > + if (IS_ERR(base))
> > + return PTR_ERR(base);
> > +
> > + dsi->regmap =
> > + devm_regmap_init_mmio(dsi->dev, base, &nwl_dsi_regmap_config);
> > + if (IS_ERR(dsi->regmap)) {
> > + ret = PTR_ERR(dsi->regmap);
> > + DRM_DEV_ERROR(dsi->dev,
> > + "Failed to create NWL DSI regmap: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + dsi->irq = platform_get_irq(pdev, 0);
> > + if (dsi->irq < 0) {
> > + DRM_DEV_ERROR(dsi->dev, "Failed to get device IRQ: %d\n",
> > + dsi->irq);
> > + return dsi->irq;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int imx8mq_dsi_poweron(struct imx_nwl_dsi *dsi)
> > +{
> > + /* otherwise the display stays blank */
> > + usleep_range(200, 300);
> > +
> > + regmap_update_bits(dsi->reset, SRC_MIPIPHY_RCR, PCLK_RESET_N,
> > + PCLK_RESET_N);
> > + regmap_update_bits(dsi->reset, SRC_MIPIPHY_RCR, ESC_RESET_N,
> > + ESC_RESET_N);
> > + regmap_update_bits(dsi->reset, SRC_MIPIPHY_RCR, RESET_BYTE_N,
> > + RESET_BYTE_N);
> > + regmap_update_bits(dsi->reset, SRC_MIPIPHY_RCR, DPI_RESET_N,
> > + DPI_RESET_N);
> > +
> > + return 0;
> > +}
> > +
> > +static int imx8mq_dsi_poweroff(struct imx_nwl_dsi *dsi)
> > +{
> > + if (USE_SRC_RESET_QUIRK(dsi->quirks))
> > + return 0;
> > +
> > + regmap_update_bits(dsi->reset, SRC_MIPIPHY_RCR, PCLK_RESET_N, 0);
> > + regmap_update_bits(dsi->reset, SRC_MIPIPHY_RCR, ESC_RESET_N, 0);
> > + regmap_update_bits(dsi->reset, SRC_MIPIPHY_RCR, RESET_BYTE_N, 0);
> > + regmap_update_bits(dsi->reset, SRC_MIPIPHY_RCR, DPI_RESET_N, 0);
> > + return 0;
> > +}
> > +
> > +static const struct drm_bridge_timings imx_nwl_timings = {
> > + .input_bus_flags = DRM_BUS_FLAG_DE_LOW,
> > +};
> > +
> > +static struct imx_nwl_platform_data imx8mq_dev = {
>
> This structure should be const, especially as it contains function
> pointers.
Done.
>
> > + .poweron = &imx8mq_dsi_poweron,
> > + .poweroff = &imx8mq_dsi_poweroff,
> > + .clk_config = {
> > + { .id = CLK_CORE, .present = true },
> > + { .id = CLK_PIXEL, .present = false },
> > + { .id = CLK_BYPASS, .present = false },
> > + },
> > + .ext_regs = IMX_REG_SRC | IMX_REG_GPR,
> > +};
> > +
> > +static const struct of_device_id imx_nwl_dsi_dt_ids[] = {
> > + { .compatible = "fsl,imx8mq-nwl-dsi", .data = &imx8mq_dev, },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, imx_nwl_dsi_dt_ids);
> > +
> > +static const struct soc_device_attribute imx_nwl_quirks_match[] = {
> > + { .soc_id = "i.MX8MQ", .revision = "2.0",
> > + .data = (void *)(E11418_HS_MODE_QUIRK | SRC_RESET_QUIRK) },
> > + { /* sentinel. */ },
> > +};
> > +
> > +static int imx_nwl_dsi_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + const struct of_device_id *of_id =
> > + of_match_device(imx_nwl_dsi_dt_ids, dev);
> > + const struct imx_nwl_platform_data *pdata = of_id->data;
> > + const struct soc_device_attribute *attr;
> > + struct imx_nwl_dsi *dsi;
> > + int ret;
> > +
> > + dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
> > + if (!dsi)
> > + return -ENOMEM;
> > +
> > + dsi->dev = dev;
> > + dsi->pdata = pdata;
> > + dsi->dpms_mode = DRM_MODE_DPMS_OFF;
>
> DPMS is legacy, let's not use it within the driver.
Removed that since the drm layer keeps the state for us.
> > +
> > + ret = imx_nwl_dsi_parse_dt(dsi);
> > + if (ret)
> > + return ret;
> > +
> > + ret = devm_request_irq(dev, dsi->irq, nwl_dsi_irq_handler, 0,
> > + dev_name(dev), dsi);
> > + if (ret < 0) {
> > + DRM_DEV_ERROR(dev, "Failed to request IRQ: %d (%d)\n", dsi->irq,
> > + ret);
> > + return ret;
> > + }
> > +
> > + dsi->dsi_host.ops = &nwl_dsi_host_ops;
> > + dsi->dsi_host.dev = dev;
> > + ret = mipi_dsi_host_register(&dsi->dsi_host);
> > + if (ret) {
> > + DRM_DEV_ERROR(dev, "Failed to register MIPI host: %d\n", ret);
> > + goto err_cleanup;
> > + }
> > +
> > + attr = soc_device_match(imx_nwl_quirks_match);
> > + if (attr)
> > + dsi->quirks = (uintptr_t)attr->data;
> > +
> > + dsi->bridge.driver_private = dsi;
> > + dsi->bridge.funcs = &imx_nwl_dsi_bridge_funcs;
> > + dsi->bridge.of_node = dev->of_node;
> > + dsi->bridge.timings = &imx_nwl_timings;
> > +
> > + drm_bridge_add(&dsi->bridge);
> > +
> > + dev_set_drvdata(dev, dsi);
> > + pm_runtime_enable(dev);
> > + return 0;
> > +
> > +err_cleanup:
> > + devm_free_irq(dev, dsi->irq, dsi);
> > + return ret;
> > +}
> > +
> > +static int imx_nwl_dsi_remove(struct platform_device *pdev)
> > +{
> > + pm_runtime_disable(&pdev->dev);
>
> You should call drm_bridge_remove() here, not in
> nwl_dsi_host_detach().
I opted to call `mipi_dsi_host_unregister(&dsi->dsi_host)` which would
keep the removal in `nwl_dsi_host_detach()` but would also make sure
it's called on removal (modelled like cdns-dsi).
>
> > + return 0;
> > +}
> > +
> > +static struct platform_driver imx_nwl_dsi_driver = {
> > + .probe = imx_nwl_dsi_probe,
> > + .remove = imx_nwl_dsi_remove,
> > + .driver = {
> > + .of_match_table = imx_nwl_dsi_dt_ids,
> > + .name = DRV_NAME,
> > + },
> > +};
> > +
> > +module_platform_driver(imx_nwl_dsi_driver);
> > +
> > +MODULE_AUTHOR("NXP Semiconductor");
> > +MODULE_AUTHOR("Purism SPC");
> > +MODULE_DESCRIPTION("i.MX8 Northwest Logic MIPI-DSI driver");
> > +MODULE_LICENSE("GPL"); /* GPLv2 or later */
> > diff --git a/drivers/gpu/drm/bridge/imx-nwl/nwl-drv.h b/drivers/gpu/drm/bridge/imx-nwl/nwl-drv.h
> > new file mode 100644
> > index 000000000000..a1e30c58b627
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/imx-nwl/nwl-drv.h
> > @@ -0,0 +1,72 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * i.MX8 NWL MIPI DSI host driver
> > + *
> > + * Copyright (C) 2017 NXP
> > + * Copyright (C) 2019 Purism SPC
> > + */
> > +
> > +#ifndef __NWL_DRV_H__
> > +#define __NWL_DRV_H__
> > +
> > +#include <drm/drm_mipi_dsi.h>
> > +#include <linux/phy/phy.h>
> > +
> > +struct imx_nwl_platform_data;
> > +
> > +/* i.MX8 NWL quirks */
> > +/* i.MX8MQ errata E11418 */
> > +#define E11418_HS_MODE_QUIRK BIT(0)
> > +#define USE_E11418_HS_MODE_QUIRK(x) ((x) & E11418_HS_MODE_QUIRK)
> > +
> > +/* Skip DSI bits in SRC on disable to avoid blank display on enable */
> > +#define SRC_RESET_QUIRK BIT(1)
> > +#define USE_SRC_RESET_QUIRK(x) ((x) & SRC_RESET_QUIRK)
>
> The USE_* macros are not shorter to type, so I would type out the &
> check explicitly.
Dropped.
> > +
> > +#define NWL_MAX_PLATFORM_CLOCKS 3
> > +struct imx_nwl_clk_config {
> > + const char *id;
> > + struct clk *clk;
> > + bool present;
> > + bool enabled;
> > + u32 rate;
> > +};
> > +
> > +struct imx_nwl_dsi {
> > + struct drm_bridge bridge;
> > + struct mipi_dsi_host dsi_host;
> > + struct drm_bridge *panel_bridge;
> > + struct device *dev;
> > + struct phy *phy;
> > + union phy_configure_opts phy_cfg;
> > + unsigned int quirks;
> > +
> > + struct regmap *regmap;
> > + int irq;
> > +
> > + /* External registers */
> > + struct regmap *csr;
> > + struct regmap *mux_sel;
> > + struct regmap *reset;
> > +
> > + /* Platform dependent clocks */
> > + struct imx_nwl_clk_config clk_config[3];
>
> I would use NWL_MAX_PLATFORM_CLOCKS instead of 3 as
> imx_nwl_platform_data uses the macro.
That was an omission, fixed.
>
> > + /* DSI clocks */
> > + struct clk *phy_ref_clk;
> > + struct clk *rx_esc_clk;
> > + struct clk *tx_esc_clk;
> > +
> > + /* dsi lanes */
> > + u32 lanes;
> > + enum mipi_dsi_pixel_format format;
> > + struct videomode vm;
> > + unsigned long dsi_mode_flags;
> > +
> > + int dpms_mode;
> > +
> > + struct mipi_dsi_transfer *xfer;
> > +
> > + const struct imx_nwl_platform_data *pdata;
> > +};
> > +
> > +#endif /* __NWL_DRV_H__ */
> > diff --git a/drivers/gpu/drm/bridge/imx-nwl/nwl-dsi.c b/drivers/gpu/drm/bridge/imx-nwl/nwl-dsi.c
> > new file mode 100644
> > index 000000000000..0e1463af162f
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/imx-nwl/nwl-dsi.c
> > @@ -0,0 +1,745 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * NWL DSI host driver
> > + *
> > + * Copyright (C) 2017 NXP
> > + * Copyright (C) 2019 Purism SPC
> > + */
> > +
> > +#include <asm/unaligned.h>
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_crtc_helper.h>
> > +#include <drm/drm_of.h>
> > +#include <drm/drm_panel.h>
> > +#include <drm/drm_print.h>
> > +#include <linux/clk.h>
> > +#include <linux/irq.h>
> > +#include <linux/regmap.h>
> > +#include <video/mipi_display.h>
> > +#include <video/videomode.h>
> > +
> > +#include "nwl-drv.h"
> > +#include "nwl-dsi.h"
> > +
> > +#define MIPI_FIFO_TIMEOUT msecs_to_jiffies(500)
> > +
> > +/* PKT reg bit manipulation */
> > +#define REG_MASK(e, s) (((1 << ((e) - (s) + 1)) - 1) << (s))
> > +#define REG_PUT(x, e, s) (((x) << (s)) & REG_MASK(e, s))
> > +#define REG_GET(x, e, s) (((x) & REG_MASK(e, s)) >> (s))
>
> Let's not reinvent the wheel, linux/bits.h and linux/bitfield.h can be
> used instead.
Done.
>
> > +
> > +/*
> > + * PKT_CONTROL format:
> > + * [15: 0] - word count
> > + * [17:16] - virtual channel
> > + * [23:18] - data type
> > + * [24] - LP or HS select (0 - LP, 1 - HS)
> > + * [25] - perform BTA after packet is sent
> > + * [26] - perform BTA only, no packet tx
> > + */
> > +#define WC(x) REG_PUT((x), 15, 0)
> > +#define TX_VC(x) REG_PUT((x), 17, 16)
> > +#define TX_DT(x) REG_PUT((x), 23, 18)
> > +#define HS_SEL(x) REG_PUT((x), 24, 24)
> > +#define BTA_TX(x) REG_PUT((x), 25, 25)
> > +#define BTA_NO_TX(x) REG_PUT((x), 26, 26)
> > +
> > +/*
> > + * RX_PKT_HEADER format:
> > + * [15: 0] - word count
> > + * [21:16] - data type
> > + * [23:22] - virtual channel
> > + */
> > +#define RX_DT(x) REG_GET((x), 21, 16)
> > +#define RX_VC(x) REG_GET((x), 23, 22)
> > +
> > +/*
> > + * DSI Video mode
> > + */
> > +#define VIDEO_MODE_BURST_MODE_WITH_SYNC_PULSES 0
> > +#define VIDEO_MODE_NON_BURST_MODE_WITH_SYNC_EVENTS BIT(0)
> > +#define VIDEO_MODE_BURST_MODE BIT(1)
> > +
> > +/*
> > + * DPI color coding
> > + */
> > +#define DPI_16_BIT_565_PACKED 0
> > +#define DPI_16_BIT_565_ALIGNED 1
> > +#define DPI_16_BIT_565_SHIFTED 2
> > +#define DPI_18_BIT_PACKED 3
> > +#define DPI_18_BIT_ALIGNED 4
> > +#define DPI_24_BIT 5
> > +
> > +/*
> > + * DPI Pixel format
> > + */
> > +#define PIXEL_FORMAT_16 0
> > +#define PIXEL_FORMAT_18 BIT(0)
> > +#define PIXEL_FORMAT_18L BIT(1)
> > +#define PIXEL_FORMAT_24 (BIT(0) | BIT(1))
> > +
> > +enum transfer_direction { DSI_PACKET_SEND, DSI_PACKET_RECEIVE };
>
> Line breaks please.
Done.
>
> > +
> > +struct mipi_dsi_transfer {
>
> Let's not use such a generic name for a driver-specific structure. You
> should name is nwl_dsi_transfer.
Fixed.
>
> > + const struct mipi_dsi_msg *msg;
> > + struct mipi_dsi_packet packet;
> > + struct completion completed;
> > +
> > + int status; /* status of transmission */
> > + enum transfer_direction direction;
> > + bool need_bta;
> > + u8 cmd;
> > + u16 rx_word_count;
> > + size_t tx_len; /* bytes sent */
> > + size_t rx_len; /* bytes received */
> > +};
> > +
> > +static inline int nwl_dsi_write(struct imx_nwl_dsi *dsi, unsigned int reg,
> > + u32 val)
> > +{
> > + int ret;
> > +
> > + ret = regmap_write(dsi->regmap, reg, val);
> > + if (ret < 0)
> > + DRM_DEV_ERROR(dsi->dev,
> > + "Failed to write NWL DSI reg 0x%x: %d\n", reg,
> > + ret);
> > + return ret;
> > +}
> > +
> > +static inline u32 nwl_dsi_read(struct imx_nwl_dsi *dsi, u32 reg)
> > +{
> > + unsigned int val;
> > + int ret;
> > +
> > + ret = regmap_read(dsi->regmap, reg, &val);
> > + if (ret < 0)
> > + DRM_DEV_ERROR(dsi->dev, "Failed to read NWL DSI reg 0x%x: %d\n",
> > + reg, ret);
> > +
> > + return val;
>
> You're loosing the error...
Looking at other drivers they often just ignore the return value of
regmap_read. The places we use this can't do much to recover from errors
so i figured it'd be best to at least log it so it becomes debuggable
instead of just dropping it to the floor.
> > +}
> > +
> > +static u32 nwl_dsi_get_dpi_pixel_format(enum mipi_dsi_pixel_format format)
> > +{
> > + switch (format) {
> > + case MIPI_DSI_FMT_RGB565:
> > + return PIXEL_FORMAT_16;
> > + case MIPI_DSI_FMT_RGB666:
> > + return PIXEL_FORMAT_18L;
> > + case MIPI_DSI_FMT_RGB666_PACKED:
> > + return PIXEL_FORMAT_18;
> > + case MIPI_DSI_FMT_RGB888:
> > + return PIXEL_FORMAT_24;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +int nwl_dsi_get_dphy_params(struct imx_nwl_dsi *dsi,
> > + const struct drm_display_mode *mode,
> > + union phy_configure_opts *phy_opts)
> > +{
> > + unsigned long rate;
> > +
> > + if (dsi->lanes < 1 || dsi->lanes > 4)
> > + return -EINVAL;
> > +
> > + /*
> > + * So far the DPHY spec minimal timings work for both mixel
> > + * dphy and nwl dsi host
> > + */
> > + phy_mipi_dphy_get_default_config(
> > + mode->crtc_clock * 1000,
> > + mipi_dsi_pixel_format_to_bpp(dsi->format), dsi->lanes,
> > + &phy_opts->mipi_dphy);
> > + rate = clk_get_rate(dsi->tx_esc_clk);
> > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "LP clk is @%lu Hz\n", rate);
> > + phy_opts->mipi_dphy.lp_clk_rate = rate;
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(nwl_dsi_get_dphy_params);
>
> No need to export symbols, those fubctions are only meant to be called
> from within the same module.
Removed (also left overs when i had these in two modules).
>
> > +
> > +#define PSEC_PER_SEC 1000000000000LL
> > +/*
> > + * ps2bc - Picoseconds to byte clock cycles
> > + */
> > +static u32 ps2bc(struct imx_nwl_dsi *dsi, unsigned long long ps)
> > +{
> > + int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> > +
> > + return DIV_ROUND_UP(ps * dsi->vm.pixelclock * bpp,
> > + dsi->lanes * 8 * PSEC_PER_SEC);
> > +}
> > +
> > +/**
> > + * ui2bc - UI time periods to byte clock cycles
> > + */
> > +static u32 ui2bc(struct imx_nwl_dsi *dsi, unsigned long long ui)
> > +{
> > + int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> > +
> > + return DIV_ROUND_UP(ui * dsi->lanes, dsi->vm.pixelclock * bpp);
> > +}
> > +
> > +#define USEC_PER_SEC 1000000L
> > +/*
> > + * us2bc - micro seconds to lp clock cycles
> > + */
> > +static u32 us2lp(u32 lp_clk_rate, unsigned long us)
> > +{
> > + return DIV_ROUND_UP(us * lp_clk_rate, USEC_PER_SEC);
> > +}
> > +
> > +static int nwl_dsi_config_host(struct imx_nwl_dsi *dsi)
> > +{
> > + u32 cycles;
> > + struct phy_configure_opts_mipi_dphy *cfg = &dsi->phy_cfg.mipi_dphy;
> > +
> > + if (dsi->lanes < 1 || dsi->lanes > 4)
> > + return -EINVAL;
> > +
> > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "DSI Lanes %d\n", dsi->lanes);
> > + nwl_dsi_write(dsi, CFG_NUM_LANES, dsi->lanes - 1);
> > +
> > + if (dsi->dsi_mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) {
> > + nwl_dsi_write(dsi, CFG_NONCONTINUOUS_CLK, 0x01);
> > + nwl_dsi_write(dsi, CFG_AUTOINSERT_EOTP, 0x01);
> > + } else {
> > + nwl_dsi_write(dsi, CFG_NONCONTINUOUS_CLK, 0x00);
> > + nwl_dsi_write(dsi, CFG_AUTOINSERT_EOTP, 0x00);
> > + }
> > +
> > + /* values in byte clock cycles */
> > + cycles = ui2bc(dsi, cfg->clk_pre);
> > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_t_pre: 0x%x\n", cycles);
> > + nwl_dsi_write(dsi, CFG_T_PRE, cycles);
> > + cycles = ps2bc(dsi, cfg->lpx + cfg->clk_prepare + cfg->clk_zero);
> > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_tx_gap (pre): 0x%x\n", cycles);
> > + cycles += ui2bc(dsi, cfg->clk_pre);
> > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_tx_gap: 0x%x\n", cycles);
> > + nwl_dsi_write(dsi, CFG_T_POST, cycles);
> > + cycles = ps2bc(dsi, cfg->hs_exit);
> > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_tx_gap: 0x%x\n", cycles);
> > + nwl_dsi_write(dsi, CFG_TX_GAP, cycles);
> > +
> > + nwl_dsi_write(dsi, CFG_EXTRA_CMDS_AFTER_EOTP, 0x01);
> > + nwl_dsi_write(dsi, CFG_HTX_TO_COUNT, 0x00);
> > + nwl_dsi_write(dsi, CFG_LRX_H_TO_COUNT, 0x00);
> > + nwl_dsi_write(dsi, CFG_BTA_H_TO_COUNT, 0x00);
> > + /* In LP clock cycles */
> > + cycles = us2lp(cfg->lp_clk_rate, cfg->wakeup);
> > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_twakeup: 0x%x\n", cycles);
> > + nwl_dsi_write(dsi, CFG_TWAKEUP, cycles);
> > +
> > + return 0;
> > +}
> > +
> > +static int nwl_dsi_config_dpi(struct imx_nwl_dsi *dsi)
> > +{
> > + struct videomode *vm = &dsi->vm;
> > + u32 color_format, mode;
> > + bool burst_mode;
> > +
> > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "hfront_porch = %d\n", vm->hfront_porch);
> > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "hback_porch = %d\n", vm->hback_porch);
> > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "hsync_len = %d\n", vm->hsync_len);
> > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "hactive = %d\n", vm->hactive);
> > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "vfront_porch = %d\n", vm->vfront_porch);
> > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "vback_porch = %d\n", vm->vback_porch);
> > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "vsync_len = %d\n", vm->vsync_len);
> > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "vactive = %d\n", vm->vactive);
> > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "clock = %lu kHz\n",
> > + vm->pixelclock / 1000);
> > +
> > + color_format = nwl_dsi_get_dpi_pixel_format(dsi->format);
> > + if (color_format < 0) {
> > + DRM_DEV_ERROR(dsi->dev, "Invalid color format 0x%x\n",
> > + dsi->format);
> > + return color_format;
> > + }
> > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "pixel fmt = %d\n", dsi->format);
> > +
> > + nwl_dsi_write(dsi, INTERFACE_COLOR_CODING, DPI_24_BIT);
> > + nwl_dsi_write(dsi, PIXEL_FORMAT, color_format);
> > + /*
> > + * Adjusting input polarity based on the video mode results in
> > + * a black screen so always pick active low:
> > + */
> > + nwl_dsi_write(dsi, VSYNC_POLARITY, VSYNC_POLARITY_ACTIVE_LOW);
> > + nwl_dsi_write(dsi, HSYNC_POLARITY, HSYNC_POLARITY_ACTIVE_LOW);
> > +
> > + burst_mode = (dsi->dsi_mode_flags & MIPI_DSI_MODE_VIDEO_BURST) &&
> > + !(dsi->dsi_mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE);
> > +
> > + if (burst_mode) {
> > + nwl_dsi_write(dsi, VIDEO_MODE, VIDEO_MODE_BURST_MODE);
> > + nwl_dsi_write(dsi, PIXEL_FIFO_SEND_LEVEL, 256);
> > + } else {
> > + mode = ((dsi->dsi_mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) ?
> > + VIDEO_MODE_BURST_MODE_WITH_SYNC_PULSES :
> > + VIDEO_MODE_NON_BURST_MODE_WITH_SYNC_EVENTS);
> > + nwl_dsi_write(dsi, VIDEO_MODE, mode);
> > + nwl_dsi_write(dsi, PIXEL_FIFO_SEND_LEVEL, vm->hactive);
> > + }
> > +
> > + nwl_dsi_write(dsi, HFP, vm->hfront_porch);
> > + nwl_dsi_write(dsi, HBP, vm->hback_porch);
> > + nwl_dsi_write(dsi, HSA, vm->hsync_len);
> > +
> > + nwl_dsi_write(dsi, ENABLE_MULT_PKTS, 0x0);
> > + nwl_dsi_write(dsi, BLLP_MODE, 0x1);
> > + nwl_dsi_write(dsi, ENABLE_MULT_PKTS, 0x0);
> > + nwl_dsi_write(dsi, USE_NULL_PKT_BLLP, 0x0);
> > + nwl_dsi_write(dsi, VC, 0x0);
> > +
> > + nwl_dsi_write(dsi, PIXEL_PAYLOAD_SIZE, vm->hactive);
> > + nwl_dsi_write(dsi, VACTIVE, vm->vactive - 1);
> > + nwl_dsi_write(dsi, VBP, vm->vback_porch);
> > + nwl_dsi_write(dsi, VFP, vm->vfront_porch);
> > +
> > + return 0;
> > +}
> > +
> > +static int nwl_dsi_enable_tx_clock(struct imx_nwl_dsi *dsi)
> > +{
> > + struct device *dev = dsi->dev;
> > + int ret;
> > +
> > + ret = clk_prepare_enable(dsi->tx_esc_clk);
> > + if (ret < 0) {
> > + DRM_DEV_ERROR(dev, "Failed to enable tx_esc clk: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + DRM_DEV_DEBUG_DRIVER(dev, "Enabled tx_esc clk @%lu Hz\n",
> > + clk_get_rate(dsi->tx_esc_clk));
> > + return 0;
> > +}
> > +
> > +static int nwl_dsi_enable_rx_clock(struct imx_nwl_dsi *dsi)
> > +{
> > + struct device *dev = dsi->dev;
> > + int ret;
> > +
> > + ret = clk_prepare_enable(dsi->rx_esc_clk);
> > + if (ret < 0) {
> > + DRM_DEV_ERROR(dev, "Failed to enable rx_esc clk: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + DRM_DEV_DEBUG_DRIVER(dev, "Enabled rx_esc clk @%lu Hz\n",
> > + clk_get_rate(dsi->rx_esc_clk));
> > + return 0;
> > +}
> > +
> > +static void nwl_dsi_init_interrupts(struct imx_nwl_dsi *dsi)
> > +{
> > + u32 irq_enable;
> > +
> > + nwl_dsi_write(dsi, IRQ_MASK, 0xffffffff);
> > + nwl_dsi_write(dsi, IRQ_MASK2, 0x7);
> > +
> > + irq_enable = ~(u32)(TX_PKT_DONE_MASK | RX_PKT_HDR_RCVD_MASK |
> > + TX_FIFO_OVFLW_MASK | HS_TX_TIMEOUT_MASK);
> > +
> > + nwl_dsi_write(dsi, IRQ_MASK, irq_enable);
> > +}
> > +
> > +static int nwl_dsi_host_attach(struct mipi_dsi_host *dsi_host,
> > + struct mipi_dsi_device *device)
> > +{
> > + struct imx_nwl_dsi *dsi =
> > + container_of(dsi_host, struct imx_nwl_dsi, dsi_host);
> > + struct device *dev = dsi->dev;
> > + struct drm_bridge *bridge;
> > + struct drm_panel *panel;
> > + int ret;
> > +
> > + DRM_DEV_INFO(dev, "lanes=%u, format=0x%x flags=0x%lx\n", device->lanes,
> > + device->format, device->mode_flags);
> > +
> > + if (device->lanes < 1 || device->lanes > 4)
> > + return -EINVAL;
> > +
> > + dsi->lanes = device->lanes;
> > + dsi->format = device->format;
> > + dsi->dsi_mode_flags = device->mode_flags;
> > +
> > + ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 1, 0, &panel,
> > + &bridge);
> > + if (ret)
> > + return ret;
> > +
> > + if (panel) {
> > + bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_DSI);
> > + if (IS_ERR(bridge))
> > + return PTR_ERR(bridge);
> > + }
> > +
> > + dsi->panel_bridge = bridge;
> > + drm_bridge_add(&dsi->bridge);
> > +
> > + return 0;
> > +}
> > +
> > +static int nwl_dsi_host_detach(struct mipi_dsi_host *dsi_host,
> > + struct mipi_dsi_device *device)
> > +{
> > + struct imx_nwl_dsi *dsi =
> > + container_of(dsi_host, struct imx_nwl_dsi, dsi_host);
> > +
> > + drm_of_panel_bridge_remove(dsi->dev->of_node, 1, 0);
> > + drm_bridge_remove(&dsi->bridge);
> > +
> > + return 0;
> > +}
> > +
> > +static bool nwl_dsi_read_packet(struct imx_nwl_dsi *dsi, u32 status)
> > +{
> > + struct device *dev = dsi->dev;
> > + struct mipi_dsi_transfer *xfer = dsi->xfer;
> > + u8 *payload = xfer->msg->rx_buf;
> > + u32 val;
> > + u16 word_count;
> > + u8 channel;
> > + u8 data_type;
> > +
> > + xfer->status = 0;
> > +
> > + if (xfer->rx_word_count == 0) {
> > + if (!(status & RX_PKT_HDR_RCVD))
> > + return false;
> > + /* Get the RX header and parse it */
> > + val = nwl_dsi_read(dsi, RX_PKT_HEADER);
> > + word_count = WC(val);
> > + channel = RX_VC(val);
> > + data_type = RX_DT(val);
> > +
> > + if (channel != xfer->msg->channel) {
> > + DRM_DEV_ERROR(dev,
> > + "[%02X] Channel mismatch (%u != %u)\n",
> > + xfer->cmd, channel, xfer->msg->channel);
> > + return true;
> > + }
> > +
> > + switch (data_type) {
> > + case MIPI_DSI_RX_GENERIC_SHORT_READ_RESPONSE_2BYTE:
> > + /* Fall through */
> > + case MIPI_DSI_RX_DCS_SHORT_READ_RESPONSE_2BYTE:
> > + if (xfer->msg->rx_len > 1) {
> > + /* read second byte */
> > + payload[1] = word_count >> 8;
> > + ++xfer->rx_len;
> > + }
> > + /* Fall through */
> > + case MIPI_DSI_RX_GENERIC_SHORT_READ_RESPONSE_1BYTE:
> > + /* Fall through */
> > + case MIPI_DSI_RX_DCS_SHORT_READ_RESPONSE_1BYTE:
> > + if (xfer->msg->rx_len > 0) {
> > + /* read first byte */
> > + payload[0] = word_count & 0xff;
> > + ++xfer->rx_len;
> > + }
> > + xfer->status = xfer->rx_len;
> > + return true;
> > + case MIPI_DSI_RX_ACKNOWLEDGE_AND_ERROR_REPORT:
> > + word_count &= 0xff;
> > + DRM_DEV_ERROR(dev, "[%02X] DSI error report: 0x%02x\n",
> > + xfer->cmd, word_count);
> > + xfer->status = -EPROTO;
> > + return true;
> > + }
> > +
> > + if (word_count > xfer->msg->rx_len) {
> > + DRM_DEV_ERROR(
> > + dev,
> > + "[%02X] Receive buffer too small: %lu (< %u)\n",
> > + xfer->cmd, xfer->msg->rx_len, word_count);
> > + return true;
> > + }
> > +
> > + xfer->rx_word_count = word_count;
> > + } else {
> > + /* Set word_count from previous header read */
> > + word_count = xfer->rx_word_count;
> > + }
> > +
> > + /* If RX payload is not yet received, wait for it */
> > + if (!(status & RX_PKT_PAYLOAD_DATA_RCVD))
> > + return false;
> > +
> > + /* Read the RX payload */
> > + while (word_count >= 4) {
> > + val = nwl_dsi_read(dsi, RX_PAYLOAD);
> > + payload[0] = (val >> 0) & 0xff;
> > + payload[1] = (val >> 8) & 0xff;
> > + payload[2] = (val >> 16) & 0xff;
> > + payload[3] = (val >> 24) & 0xff;
> > + payload += 4;
> > + xfer->rx_len += 4;
> > + word_count -= 4;
> > + }
> > +
> > + if (word_count > 0) {
> > + val = nwl_dsi_read(dsi, RX_PAYLOAD);
> > + switch (word_count) {
> > + case 3:
> > + payload[2] = (val >> 16) & 0xff;
> > + ++xfer->rx_len;
> > + /* Fall through */
> > + case 2:
> > + payload[1] = (val >> 8) & 0xff;
> > + ++xfer->rx_len;
> > + /* Fall through */
> > + case 1:
> > + payload[0] = (val >> 0) & 0xff;
> > + ++xfer->rx_len;
> > + break;
> > + }
> > + }
> > +
> > + xfer->status = xfer->rx_len;
> > +
> > + return true;
> > +}
> > +
> > +static void nwl_dsi_finish_transmission(struct imx_nwl_dsi *dsi, u32 status)
> > +{
> > + struct mipi_dsi_transfer *xfer = dsi->xfer;
> > + bool end_packet = false;
> > +
> > + if (!xfer)
> > + return;
> > +
> > + if (status & TX_FIFO_OVFLW) {
> > + DRM_DEV_ERROR_RATELIMITED(dsi->dev, "tx fifo overflow\n");
> > + return;
> > + }
> > +
> > + if (status & HS_TX_TIMEOUT) {
> > + DRM_DEV_ERROR_RATELIMITED(dsi->dev, "HS tx timeout\n");
> > + return;
> > + }
> > +
> > + if (xfer->direction == DSI_PACKET_SEND && status & TX_PKT_DONE) {
> > + xfer->status = xfer->tx_len;
> > + end_packet = true;
> > + } else if (status & DPHY_DIRECTION &&
> > + ((status & (RX_PKT_HDR_RCVD | RX_PKT_PAYLOAD_DATA_RCVD)))) {
> > + end_packet = nwl_dsi_read_packet(dsi, status);
> > + }
> > +
> > + if (end_packet)
> > + complete(&xfer->completed);
> > +}
> > +
> > +static void nwl_dsi_begin_transmission(struct imx_nwl_dsi *dsi)
> > +{
> > + struct mipi_dsi_transfer *xfer = dsi->xfer;
> > + struct mipi_dsi_packet *pkt = &xfer->packet;
> > + const u8 *payload;
> > + size_t length;
> > + u16 word_count;
> > + u8 hs_mode;
> > + u32 val;
> > + u32 hs_workaround = 0;
> > +
> > + /* Send the payload, if any */
> > + length = pkt->payload_length;
> > + payload = pkt->payload;
> > +
> > + while (length >= 4) {
> > + val = get_unaligned_le32(payload);
>
> The framework doesn't guarantee the payload to be aligned on a multiple
> of 4 bytes ?
It does as far as i can tell, dropped.
>
> > + hs_workaround |= !(val & 0xFFFF00);
> > + nwl_dsi_write(dsi, TX_PAYLOAD, val);
> > + payload += 4;
> > + length -= 4;
> > + }
> > + /* Send the rest of the payload */
> > + val = 0;
> > + switch (length) {
> > + case 3:
> > + val |= payload[2] << 16;
> > + /* Fall through */
> > + case 2:
> > + val |= payload[1] << 8;
> > + hs_workaround |= !(val & 0xFFFF00);
> > + /* Fall through */
> > + case 1:
> > + val |= payload[0];
> > + nwl_dsi_write(dsi, TX_PAYLOAD, val);
> > + break;
> > + }
> > + xfer->tx_len = pkt->payload_length;
> > +
> > + /*
> > + * Send the header
> > + * header[0] = Virtual Channel + Data Type
> > + * header[1] = Word Count LSB (LP) or first param (SP)
> > + * header[2] = Word Count MSB (LP) or second param (SP)
> > + */
> > + word_count = pkt->header[1] | (pkt->header[2] << 8);
> > + if ((hs_workaround && USE_E11418_HS_MODE_QUIRK(dsi->quirks))) {
> > + DRM_DEV_DEBUG_DRIVER(dsi->dev,
> > + "Using hs mode workaround for cmd 0x%x\n",
> > + xfer->cmd);
> > + hs_mode = 1;
> > + } else {
> > + hs_mode = (xfer->msg->flags & MIPI_DSI_MSG_USE_LPM) ? 0 : 1;
> > + }
> > + val = WC(word_count) |
> > + TX_VC(xfer->msg->channel) |
> > + TX_DT(xfer->msg->type) |
> > + HS_SEL(hs_mode) |
> > + BTA_TX(xfer->need_bta);
> > + nwl_dsi_write(dsi, PKT_CONTROL, val);
> > +
> > + /* Send packet command */
> > + nwl_dsi_write(dsi, SEND_PACKET, 0x1);
> > +}
> > +
> > +static ssize_t nwl_dsi_host_transfer(struct mipi_dsi_host *dsi_host,
> > + const struct mipi_dsi_msg *msg)
> > +{
> > + struct imx_nwl_dsi *dsi =
> > + container_of(dsi_host, struct imx_nwl_dsi, dsi_host);
> > + struct mipi_dsi_transfer xfer;
> > + ssize_t ret = 0;
> > +
> > + /* Create packet to be sent */
> > + dsi->xfer = &xfer;
> > + ret = mipi_dsi_create_packet(&xfer.packet, msg);
> > + if (ret < 0) {
> > + dsi->xfer = NULL;
> > + return ret;
> > + }
> > +
> > + if ((msg->type & MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM ||
> > + msg->type & MIPI_DSI_GENERIC_READ_REQUEST_1_PARAM ||
> > + msg->type & MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM ||
> > + msg->type & MIPI_DSI_DCS_READ) &&
> > + msg->rx_len > 0 && msg->rx_buf != NULL)
> > + xfer.direction = DSI_PACKET_RECEIVE;
> > + else
> > + xfer.direction = DSI_PACKET_SEND;
> > +
> > + xfer.need_bta = (xfer.direction == DSI_PACKET_RECEIVE);
> > + xfer.need_bta |= (msg->flags & MIPI_DSI_MSG_REQ_ACK) ? 1 : 0;
> > + xfer.msg = msg;
> > + xfer.status = -ETIMEDOUT;
> > + xfer.rx_word_count = 0;
> > + xfer.rx_len = 0;
> > + xfer.cmd = 0x00;
> > + if (msg->tx_len > 0)
> > + xfer.cmd = ((u8 *)(msg->tx_buf))[0];
> > + init_completion(&xfer.completed);
> > +
> > + nwl_dsi_enable_rx_clock(dsi);
> > +
> > + /* Initiate the DSI packet transmision */
> > + nwl_dsi_begin_transmission(dsi);
> > +
> > + if (!wait_for_completion_timeout(&xfer.completed, MIPI_FIFO_TIMEOUT)) {
> > + DRM_DEV_ERROR(dsi_host->dev, "[%02X] DSI transfer timed out\n",
> > + xfer.cmd);
> > + ret = -ETIMEDOUT;
> > + } else {
> > + ret = xfer.status;
> > + }
> > +
> > + clk_disable_unprepare(dsi->rx_esc_clk);
> > +
> > + return ret;
> > +}
> > +
> > +const struct mipi_dsi_host_ops nwl_dsi_host_ops = {
> > + .attach = nwl_dsi_host_attach,
> > + .detach = nwl_dsi_host_detach,
> > + .transfer = nwl_dsi_host_transfer,
> > +};
> > +
> > +irqreturn_t nwl_dsi_irq_handler(int irq, void *data)
> > +{
> > + u32 irq_status;
> > + struct imx_nwl_dsi *dsi = data;
> > +
> > + irq_status = nwl_dsi_read(dsi, IRQ_STATUS);
> > +
> > + if (irq_status & TX_PKT_DONE || irq_status & RX_PKT_HDR_RCVD ||
> > + irq_status & RX_PKT_PAYLOAD_DATA_RCVD)
> > + nwl_dsi_finish_transmission(dsi, irq_status);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +EXPORT_SYMBOL_GPL(nwl_dsi_irq_handler);
> > +
> > +int nwl_dsi_enable(struct imx_nwl_dsi *dsi)
> > +{
> > + struct device *dev = dsi->dev;
> > + union phy_configure_opts *phy_cfg = &dsi->phy_cfg;
> > + int ret;
> > +
> > + if (!dsi->lanes) {
> > + DRM_DEV_ERROR(dev, "Need DSI lanes: %d\n", dsi->lanes);
> > + return -EINVAL;
> > + }
> > +
> > + ret = phy_init(dsi->phy);
> > + if (ret < 0) {
> > + DRM_DEV_ERROR(dev, "Failed to init DSI phy: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = phy_configure(dsi->phy, phy_cfg);
> > + if (ret < 0) {
> > + DRM_DEV_ERROR(dev, "Failed to configure DSI phy: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = nwl_dsi_enable_tx_clock(dsi);
> > + if (ret < 0) {
> > + DRM_DEV_ERROR(dev, "Failed to enable tx clock: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = nwl_dsi_config_host(dsi);
> > + if (ret < 0) {
> > + DRM_DEV_ERROR(dev, "Failed to set up DSI: %d", ret);
> > + return ret;
> > + }
> > +
> > + ret = nwl_dsi_config_dpi(dsi);
> > + if (ret < 0) {
> > + DRM_DEV_ERROR(dev, "Failed to set up DPI: %d", ret);
> > + return ret;
> > + }
> > +
> > + ret = phy_power_on(dsi->phy);
> > + if (ret < 0) {
> > + DRM_DEV_ERROR(dev, "Failed to power on DPHY (%d)\n", ret);
> > + return ret;
> > + }
> > +
> > + nwl_dsi_init_interrupts(dsi);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(nwl_dsi_enable);
> > +
> > +int nwl_dsi_disable(struct imx_nwl_dsi *dsi)
> > +{
> > + struct device *dev = dsi->dev;
> > +
> > + DRM_DEV_DEBUG_DRIVER(dev, "Disabling clocks and phy\n");
> > +
> > + phy_power_off(dsi->phy);
> > + phy_exit(dsi->phy);
> > +
> > + /* Disabling the clock before the phy breaks enabling dsi again */
> > + clk_disable_unprepare(dsi->tx_esc_clk);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(nwl_dsi_disable);
> > diff --git a/drivers/gpu/drm/bridge/imx-nwl/nwl-dsi.h b/drivers/gpu/drm/bridge/imx-nwl/nwl-dsi.h
> > new file mode 100644
> > index 000000000000..7bcf804843e2
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/imx-nwl/nwl-dsi.h
> > @@ -0,0 +1,111 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * i.MX8 NWL MIPI DSI host driver
> > + *
> > + * Copyright (C) 2017 NXP
> > + * Copyright (C) 2019 Purism SPC
> > + */
> > +#ifndef __NWL_DSI_H__
> > +#define __NWL_DSI_H__
> > +
> > +#include <drm/drm_mipi_dsi.h>
> > +
> > +/* DSI HOST registers */
> > +#define CFG_NUM_LANES 0x0
>
> Some of the register names are quite prone to namespace clashes. I
> recommend prefixing them all with NWL_DSI_.
Done, also the defines in nwl-dsi.c.
>
> > +#define CFG_NONCONTINUOUS_CLK 0x4
> > +#define CFG_T_PRE 0x8
> > +#define CFG_T_POST 0xc
> > +#define CFG_TX_GAP 0x10
> > +#define CFG_AUTOINSERT_EOTP 0x14
> > +#define CFG_EXTRA_CMDS_AFTER_EOTP 0x18
> > +#define CFG_HTX_TO_COUNT 0x1c
> > +#define CFG_LRX_H_TO_COUNT 0x20
> > +#define CFG_BTA_H_TO_COUNT 0x24
> > +#define CFG_TWAKEUP 0x28
> > +#define CFG_STATUS_OUT 0x2c
> > +#define RX_ERROR_STATUS 0x30
> > +
> > +/* DSI DPI registers */
> > +#define PIXEL_PAYLOAD_SIZE 0x200
> > +#define PIXEL_FIFO_SEND_LEVEL 0x204
> > +#define INTERFACE_COLOR_CODING 0x208
> > +#define PIXEL_FORMAT 0x20c
> > +#define VSYNC_POLARITY 0x210
> > +#define VSYNC_POLARITY_ACTIVE_LOW 0
> > +#define VSYNC_POLARITY_ACTIVE_HIGH BIT(1)
> > +
> > +#define HSYNC_POLARITY 0x214
> > +#define HSYNC_POLARITY_ACTIVE_LOW 0
> > +#define HSYNC_POLARITY_ACTIVE_HIGH BIT(1)
> > +
> > +#define VIDEO_MODE 0x218
> > +#define HFP 0x21c
> > +#define HBP 0x220
> > +#define HSA 0x224
> > +#define ENABLE_MULT_PKTS 0x228
> > +#define VBP 0x22c
> > +#define VFP 0x230
> > +#define BLLP_MODE 0x234
> > +#define USE_NULL_PKT_BLLP 0x238
> > +#define VACTIVE 0x23c
> > +#define VC 0x240
> > +
> > +/* DSI APB PKT control */
> > +#define TX_PAYLOAD 0x280
> > +#define PKT_CONTROL 0x284
> > +#define SEND_PACKET 0x288
> > +#define PKT_STATUS 0x28c
> > +#define PKT_FIFO_WR_LEVEL 0x290
> > +#define PKT_FIFO_RD_LEVEL 0x294
> > +#define RX_PAYLOAD 0x298
> > +#define RX_PKT_HEADER 0x29c
> > +
> > +/* DSI IRQ handling */
> > +#define IRQ_STATUS 0x2a0
> > +#define SM_NOT_IDLE BIT(0)
> > +#define TX_PKT_DONE BIT(1)
> > +#define DPHY_DIRECTION BIT(2)
> > +#define TX_FIFO_OVFLW BIT(3)
> > +#define TX_FIFO_UDFLW BIT(4)
> > +#define RX_FIFO_OVFLW BIT(5)
> > +#define RX_FIFO_UDFLW BIT(6)
> > +#define RX_PKT_HDR_RCVD BIT(7)
> > +#define RX_PKT_PAYLOAD_DATA_RCVD BIT(8)
> > +#define BTA_TIMEOUT BIT(29)
> > +#define LP_RX_TIMEOUT BIT(30)
> > +#define HS_TX_TIMEOUT BIT(31)
> > +
> > +#define IRQ_STATUS2 0x2a4
> > +#define SINGLE_BIT_ECC_ERR BIT(0)
> > +#define MULTI_BIT_ECC_ERR BIT(1)
> > +#define CRC_ERR BIT(2)
> > +
> > +#define IRQ_MASK 0x2a8
> > +#define SM_NOT_IDLE_MASK BIT(0)
> > +#define TX_PKT_DONE_MASK BIT(1)
> > +#define DPHY_DIRECTION_MASK BIT(2)
> > +#define TX_FIFO_OVFLW_MASK BIT(3)
> > +#define TX_FIFO_UDFLW_MASK BIT(4)
> > +#define RX_FIFO_OVFLW_MASK BIT(5)
> > +#define RX_FIFO_UDFLW_MASK BIT(6)
> > +#define RX_PKT_HDR_RCVD_MASK BIT(7)
> > +#define RX_PKT_PAYLOAD_DATA_RCVD_MASK BIT(8)
> > +#define BTA_TIMEOUT_MASK BIT(29)
> > +#define LP_RX_TIMEOUT_MASK BIT(30)
> > +#define HS_TX_TIMEOUT_MASK BIT(31)
> > +
> > +#define IRQ_MASK2 0x2ac
> > +#define SINGLE_BIT_ECC_ERR_MASK BIT(0)
> > +#define MULTI_BIT_ECC_ERR_MASK BIT(1)
> > +#define CRC_ERR_MASK BIT(2)
> > +
> > +extern const struct mipi_dsi_host_ops nwl_dsi_host_ops;
> > +
> > +irqreturn_t nwl_dsi_irq_handler(int irq, void *data);
> > +int nwl_dsi_enable(struct imx_nwl_dsi *dsi);
> > +int nwl_dsi_disable(struct imx_nwl_dsi *dsi);
> > +int nwl_dsi_get_dphy_params(struct imx_nwl_dsi *dsi,
> > + const struct drm_display_mode *mode,
> > + union phy_configure_opts *phy_opts);
> > +
> > +#endif /* __NWL_DSI_H__ */
>
> --
> Regards,
>
> Laurent Pinchart
>
Cheers and thanks for having a look!
-- Guido
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox