| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <evfhjmeblrucqta2jb74jwul7evqt25tbsxp46xrghytbr645d@t6rvyuriruax>
Date: Thu, 3 Oct 2024 00:05:52 +0200
From: Andi Shyti <andi.shyti@...nel.org>
To: carlos.song@....com
Cc: aisheng.dong@....com, shawnguo@...nel.org, s.hauer@...gutronix.de,
kernel@...gutronix.de, festevam@...il.com, frank.li@....com, linux-i2c@...r.kernel.org,
imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH V4] i2c: imx-lpi2c: add target mode support
Hi Carlos,
On Thu, Sep 12, 2024 at 04:24:13PM GMT, carlos.song@....com wrote:
> From: Carlos Song <carlos.song@....com>
>
> LPI2C support master controller and target controller enabled
> simultaneously. Both controllers share same SDA/SCL lines and
/same/the same/
> interrupt source but has separate control and status registers.
/separate/a separate/
> Even if target mode is enabled, LPI2C can still work normally
> as master controller at the same time.
It's not what happens in the irq handler, though (I left a
comment in irq handler).
> This patch supports basic target data read/write operations in
> 7-bit target address. LPI2C target mode can be enabled by using
> I2C slave backend. I2C slave backend behave like a standard I2C
/behave/behaves/
> client. For simple use and test, Linux I2C slave EEPROM backend
> can be used.
>
> Signed-off-by: Carlos Song <carlos.song@....com>
...
> +static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id)
> +{
> + struct lpi2c_imx_struct *lpi2c_imx = dev_id;
> +
> + if (lpi2c_imx->target) {
> + u32 scr = readl(lpi2c_imx->base + LPI2C_SCR);
> + u32 ssr = readl(lpi2c_imx->base + LPI2C_SSR);
> + u32 sier_filter = ssr & readl(lpi2c_imx->base + LPI2C_SIER);
> +
> + /* Target is enabled and trigger irq then enter target irq handler */
This sentence is a bit hard to understand, how about:
/*
* The target is enabled and an interrupt has
* been triggered. Enter the target's irq handler.
*/
> + if ((scr & SCR_SEN) && sier_filter)
> + return lpi2c_imx_target_isr(lpi2c_imx, ssr, sier_filter);
Can't the interrupt be generated by the master if
lpi2c_imx->target is assigned?
In the git log you are describing a different behavior.
> + }
> +
> + /* Otherwise triggered by master then handle irq in master handler */
Otherwise the interrupt has been triggered by the master. Enter
the master's irq handler.
> + return lpi2c_imx_master_isr(lpi2c_imx);
> +}
> +
> +static void lpi2c_imx_target_init(struct lpi2c_imx_struct *lpi2c_imx)
> +{
> + u32 temp;
> +
> + /* reset target module */
> + writel(SCR_RST, lpi2c_imx->base + LPI2C_SCR);
> + writel(0, lpi2c_imx->base + LPI2C_SCR);
> +
> + /* Set target addr */
/addr/address/
> + writel((lpi2c_imx->target->addr << 1), lpi2c_imx->base + LPI2C_SAMR);
> +
> + writel(SCFGR1_RXSTALL | SCFGR1_TXDSTALL, lpi2c_imx->base + LPI2C_SCFGR1);
> +
> + /*
> + * set SCFGR2: FILTSDA, FILTSCL and CLKHOLD
> + *
> + * FILTSCL/FILTSDA can eliminate signal skew. It should generally be
> + * set to the same value and should be set >= 50ns.
> + *
> + * CLKHOLD is only used when clock stretching is enabled, but it will
> + * extend the clock stretching to ensure there is an additional delay
> + * between the target driving SDA and the target releasing the SCL pin.
> + *
> + * CLKHOLD setting is crucial for lpi2c target. When master read data
> + * from target, if there is a delay caused by cpu idle, excessive load,
> + * or other delays between two bytes in one message transmission. so it
> + * will cause a short interval time between the driving SDA signal and
/transmission. so it will/transmittion, it will/
> + * releasing SCL signal. Lpi2c master will mistakenly think it is a stop
/Lpi2c/The lpi2c/
> + * signal resulting in an arbitration failure. This issue can be avoided
> + * by setting CLKHOLD.
> + *
> + * In order to ensure lpi2c function normally when the lpi2c speed is as
> + * low as 100kHz, CLKHOLD should be set 3 and it is also compatible with
/3/to 3/
> + * higher clock frequency like 400kHz and 1MHz.
> + */
> + temp = SCFGR2_FILTSDA(2) | SCFGR2_FILTSCL(2) | SCFGR2_CLKHOLD(3);
> + writel(temp, lpi2c_imx->base + LPI2C_SCFGR2);
> +
> + /*
> + * Enable module:
> + * SCR_FILTEN can enable digital filter and output delay counter for LPI2C
> + * target mode. So SCR_FILTEN need be asserted when enable SDA/SCL FILTER
> + * and CLKHOLD.
> + */
> + writel(SCR_SEN | SCR_FILTEN, lpi2c_imx->base + LPI2C_SCR);
> +
> + /* Enable interrupt from i2c module */
> + writel(SLAVE_INT_FLAG, lpi2c_imx->base + LPI2C_SIER);
> +}
> +
> +static int lpi2c_imx_reg_target(struct i2c_client *client)
lpi2c_imx_register_target as a name is a bit better, in my opinion
> +{
> + struct lpi2c_imx_struct *lpi2c_imx = i2c_get_adapdata(client->adapter);
> + int ret;
> +
> + if (lpi2c_imx->target)
> + return -EBUSY;
> +
> + lpi2c_imx->target = client;
> +
> + ret = pm_runtime_resume_and_get(lpi2c_imx->adapter.dev.parent);
> + if (ret < 0) {
> + dev_err(&lpi2c_imx->adapter.dev, "failed to resume i2c controller");
> + return ret;
> + }
> +
> + lpi2c_imx_target_init(lpi2c_imx);
> +
> + return 0;
> +}
> +
> +static int lpi2c_imx_unreg_target(struct i2c_client *client)
lpi2c_imx_unregister_target sounds better to me.
> +{
> + struct lpi2c_imx_struct *lpi2c_imx = i2c_get_adapdata(client->adapter);
> + int ret;
> +
> + if (!lpi2c_imx->target)
> + return -EINVAL;
> +
...
> +static int lpi2c_suspend_noirq(struct device *dev)
> +{
> + int ret;
> +
> + ret = pm_runtime_force_suspend(dev);
> + if (ret)
> + return ret;
> +
> + return 0;
This function can simply be:
static int lpi2c_suspend_noirq(struct device *dev)
{
return pm_runtime_force_suspend(dev);
}
but I'm not strong for it, your choice.
> +}
> +
> +static int lpi2c_resume_noirq(struct device *dev)
> +{
> + struct lpi2c_imx_struct *lpi2c_imx = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = pm_runtime_force_resume(dev);
> + if (ret)
> + return ret;
> +
> + /*
> + * If i2c module powered down in system suspend, register
> + * value will lose. So reinit target when system resume.
> + */
Can we re-write this to something like:
/*
* If the I2C module powers down during system suspend,
* the register values will be lost. Therefore, reinitialize
* the target when the system resumes.
*/
Thanks,
Andi
> + if (lpi2c_imx->target)
> + lpi2c_imx_target_init(lpi2c_imx);
> +
> + return 0;
> +}
> +
> static const struct dev_pm_ops lpi2c_pm_ops = {
> - SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> - pm_runtime_force_resume)
> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(lpi2c_suspend_noirq,
> + lpi2c_resume_noirq)
> SET_RUNTIME_PM_OPS(lpi2c_runtime_suspend,
> lpi2c_runtime_resume, NULL)
> };
> --
> 2.34.1
>
Powered by blists - more mailing lists