* Re: [PATCH v2,04/10] media: mediatek: vcodec: remove the dependency of debug log
From: Yunfei Dong (董云飞) @ 2023-06-14 9:17 UTC (permalink / raw)
To: wenst@chromium.org, benjamin.gaignard@collabora.com,
nfraprado@collabora.com, angelogioacchino.delregno@collabora.com,
nicolas.dufresne@collabora.com, nhebert@chromium.org,
hverkuil-cisco@xs4all.nl
Cc: linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
frkoenig@chromium.org, stevecho@chromium.org,
linux-media@vger.kernel.org, devicetree@vger.kernel.org,
daniel@ffwll.ch, Project_Global_Chrome_Upstream_Group,
hsinyi@chromium.org, linux-arm-kernel@lists.infradead.org
In-Reply-To: <e1e00e3207784f48b6adc9c3b6ec48f57795228d.camel@collabora.com>
Hi AngeloGioacchino,
How do you think about Nicolas's suggestion?
On Thu, 2023-06-08 at 11:17 -0400, Nicolas Dufresne wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> Le jeudi 08 juin 2023 à 16:06 +0200, AngeloGioacchino Del Regno a
> écrit :
> > Il 08/06/23 15:11, Nicolas Dufresne ha scritto:
> > > Le jeudi 08 juin 2023 à 07:27 +0000, Yunfei Dong (董云飞) a écrit :
> > > > Hi Nicolas,
> > > >
> > > > Thanks for your review.
> > > > On Wed, 2023-06-07 at 21:41 -0400, Nicolas Dufresne wrote:
> > > > >
> > > > > External email : Please do not click links or open
> attachments until
> > > > > you have verified the sender or the content.
> > > > > Hi Yunfei,
> > > > >
> > > > > Le mercredi 07 juin 2023 à 16:48 +0800, Yunfei Dong a écrit :
> > > > > > 'mtk_vcodec_debug' and 'mtk_vcodec_err' depends on
> 'mtk_vcodec_ctx'
> > > > > > to get the index of each instance, using the index directly
> instead
> > > > > > of with 'mtk_vcodec_ctx'.
> > > > > >
> > > > > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
> > > > > > ---
> > > > > > .../mediatek/vcodec/mtk_vcodec_util.h | 26 ++-
> > > > > > .../vcodec/vdec/vdec_av1_req_lat_if.c | 105
> +++++++-----
> > > > > > .../mediatek/vcodec/vdec/vdec_h264_if.c | 62 ++++-
> --
> > > > > > .../mediatek/vcodec/vdec/vdec_h264_req_if.c | 39 +++--
> > > > > > .../vcodec/vdec/vdec_h264_req_multi_if.c | 80
> +++++----
> > > > > > .../vcodec/vdec/vdec_hevc_req_multi_if.c | 67 ++++-
> ---
> > > > > > .../mediatek/vcodec/vdec/vdec_vp8_if.c | 54 ++++-
> --
> > > > > > .../mediatek/vcodec/vdec/vdec_vp8_req_if.c | 46 +++
> ---
> > > > > > .../mediatek/vcodec/vdec/vdec_vp9_if.c | 152
> ++++++++++--
> > > > > ------
> > > > > > .../vcodec/vdec/vdec_vp9_req_lat_if.c | 84
> ++++++----
> > > > > > .../platform/mediatek/vcodec/vdec_vpu_if.c | 59 ++++-
> --
> > > > > > .../mediatek/vcodec/venc/venc_h264_if.c | 86
> +++++-----
> > > > > > .../mediatek/vcodec/venc/venc_vp8_if.c | 48 +++
> ---
> > > > > > .../platform/mediatek/vcodec/venc_vpu_if.c | 64 ++++-
> ---
> > > > > > 14 files changed, 565 insertions(+), 407 deletions(-)
> > > > > >
> > > > > > diff --git
> > > > > a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h
> > > > > b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h
> > > > > > index ecb0bdf3a4f4..ddc12c3e2983 100644
> > > > > > ---
> a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h
> > > > > > +++
> b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h
> > > > > > @@ -31,9 +31,8 @@ struct mtk_vcodec_dev;
> > > > > > #define mtk_v4l2_err(fmt, args...) \
> > > > > > pr_err("[MTK_V4L2][ERROR] " fmt "\n", ##args)
> > > > > >
> > > > > > -#define mtk_vcodec_err(h, fmt, args...)\
> > > > > > -pr_err("[MTK_VCODEC][ERROR][%d]: " fmt "\n",\
> > > > > > - ((struct mtk_vcodec_ctx *)(h)->ctx)->id, ##args)
> > > > > > +#define mtk_vcodec_err(plat_dev, inst_id, fmt,
> > > > > args...) \
> > > > > > +dev_err(&(plat_dev)->dev, "[MTK_VCODEC][ERROR][%d]: " fmt
> "\n",
> > > > > inst_id, ##args)
> > > > > >
> > > > > > #if defined(CONFIG_DEBUG_FS)
> > > > > > extern int mtk_v4l2_dbg_level;
> > > > > > @@ -46,27 +45,24 @@ extern int mtk_vcodec_dbg;
> > > > > > __func__, __LINE__, ##args); \
> > > > > > } while (0)
> > > > > >
> > > > > > -#define mtk_vcodec_debug(h, fmt,
> args...) \
> > > > > > -do { \
> > > > > > -if (mtk_vcodec_dbg) \
> > > > > > -dev_dbg(&(((struct mtk_vcodec_ctx *)(h)->ctx)->dev-
> >plat_dev-
> > > > > > dev), \
> > > > > > -"[MTK_VCODEC][%d]: %s, %d " fmt
> "\n", \
> > > > > > -((struct mtk_vcodec_ctx *)(h)->ctx)-
> >id, \
> > > > > > -__func__, __LINE__,
> ##args); \
> > > > > > +#define mtk_vcodec_debug(plat_dev, inst_id, fmt,
> > > > > args...) \
> > > > > > +do
> > > > > {
> > > > > \
> > > > > > +if
> > > > > (mtk_vcodec_dbg)
> > > > > \
> > > > > > +dev_dbg(&(plat_dev)->dev, "[MTK_VCODEC][%d]: %s, %d " fmt
> "\n", \
> > > > >
> > > > > At least in this patch, you systematically pass plat_dev as
> > > > > <something>->ctx->dev->plat_dev, which is quite long and
> verbose, any
> > > > > reason we
> > > > > can't just pass that <something> here ? We can follow the
> same
> > > > > structure path
> > > > > for both encoder/decoder ?
> > > > >
> > > >
> > > > In order to separate encode and decoder, need to define two
> different
> > > > struct mtk_vcodec_dec_ctx and struct mtk_vcodec_enc_ctx.
> > > >
> > > > struct mtk_vcodec_ctx won't be used again, need to use platform
> device
> > > > to print dev_dbg and dev_err.
> > > >
> > > > encoder and decoder using the same interface to print log
> message.
> > >
> > > Just a reminder, I'm just making suggestions, there is no strict
> action required
> > > here other then a discussion to try and make the logging a bit
> more light.
> > >
> > > My points was that C macros don't care about types, so if you
> keep the path to
> > > the platform device the same (ctx->dev->plat_dev), you could just
> pass the ctx
> > > as argument. What I don't know though myself, is if this is
> actually feasible in
> > > all code path, but considering you had access to the instance
> previously, I
> > > thought it should.
> > >
> >
> > One macro used to access two different structures?
> >
> > Please, no.
>
> Its up to you. I do think this is an empty statement. Still believe
> we avoid
> this code "deterioration". One can always be creative to workaround
> your
> concerns.
>
> struct base_ctx {
> struct dev dev;
> }
>
> struct enc_ctx {
> struct base_ctx;
> ...
> }
>
> struct src_ctx {
> ...
> }
>
> But this is in no way more safe then a naming convention, this is
> macro calls,
> its not typed.
>
> Nicolas
>
In order to speed up the upstream progress, maybe we can discuss it in
chat.
Best Reagrds,
Yunfei Dong
> >
> > Regards,
> > Angelo
> >
> > > regards,
> > > Nicolas
> > >
> > > >
> > > > Best Regards,
> > > > Yunfei Dong
> > > > > > +inst_id, __func__, __LINE__, ##args); \
> > > > > > } while (0)
> > > > > > #else
> > > > > > #define mtk_v4l2_debug(level, fmt, args...) pr_debug(fmt,
> ##args)
> > > > > >
> > > > > > -#define mtk_vcodec_debug(h, fmt, args...)\
> > > > > > -pr_debug("[MTK_VCODEC][%d]: " fmt "\n",\
> > > > >
> > > > ...snip...
> > >
> >
>
^ permalink raw reply
* Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
From: Laurent Pinchart @ 2023-06-14 9:54 UTC (permalink / raw)
To: Biju Das
Cc: Wolfram Sang, Geert Uytterhoeven, Alexandre Belloni, Mark Brown,
Krzysztof Kozlowski, Rob Herring, Conor Dooley, Andrzej Hajda,
Neil Armstrong, Robert Foss, David Airlie, Daniel Vetter,
Kieran Bingham, Mauro Carvalho Chehab, Hans Verkuil,
Alessandro Zummo, Jonas Karlman, Jernej Skrabec,
Uwe Kleine-König, Corey Minyard, Marek Behún,
Jiasheng Jiang, Antonio Borneo, Abhinav Kumar, Ahmad Fatoum,
dri-devel@lists.freedesktop.org, linux-i2c@vger.kernel.org,
linux-media@vger.kernel.org, Fabrizio Castro,
linux-renesas-soc@vger.kernel.org
In-Reply-To: <OS0PR01MB59225C45554667D342454923865AA@OS0PR01MB5922.jpnprd01.prod.outlook.com>
On Wed, Jun 14, 2023 at 08:21:38AM +0000, Biju Das wrote:
> Hi Laurent,
>
> Thanks for the feedback.
>
> > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
> >
> > On Tue, Jun 13, 2023 at 07:31:46PM +0000, Biju Das wrote:
> > > > Subject: RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device
> > > > API
> > > > > Subject: RE: [PATCH v5 01/11] i2c: Enhance
> > > > > i2c_new_ancillary_device API
> > > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance
> > > > > > i2c_new_ancillary_device API
> > > > > >
> > > > > > Hi everyone,
> > > > > >
> > > > > > > Perhaps we should first think through what an ancillary device
> > > > > > > really is. My understanding is that it is used to talk to
> > > > > > > secondary addresses of a multi-address I2C slave device.
> > > > > >
> > > > > > As I mentioned somewhere before, this is not the case. Ancillary
> > > > > > devices are when one *driver* handles more than one address.
> > > > > > Everything else has been handled differently in the past (for
> > > > > > all the uses I am aware of).
> > > > > >
> > > > > > Yet, I have another idea which is so simple that I wonder if it
> > > > > > maybe has already been discussed so far?
> > > > > >
> > > > > > * have two regs in the bindings
> > > > >
> > > > > OK, it is inline with DT maintainers expectation as it is matching
> > > > > with real hw as single device node having two regs.
> > > > >
> > > > > > * use the second reg with i2c_new_client_device to instantiate the
> > > > > > RTC sibling. 'struct i2c_board_info', which is one parameter, should
> > > > > > have enough options to pass data, e.g it has a software_node.
> > > > >
> > > > > OK, I can see the below can be passed from PMIC to new client device.
> > > > >
> > > > > client->addr = info->addr;
> > > > >
> > > > > client->init_irq = info->irq;
> > > > >
> > > > > >
> > > > > > Should work or did I miss something here?
> > > > >
> > > > > I guess it will work. We instantiate appropriate device based On
> > > > > PMIC revision and slave address and IRQ resource passed through
> > > > > 'struct i2c_board_info'
> > > > >
> > > > > Will check this and update you.
> > > >
> > > > info.irq = irq; -->Irq fine
> > > > info.addr = addr; -->slave address fine size = strscpy(info.type,
> > > > name, sizeof(info.type)); -->instantiation based on PMIC version
> > > > fine.
> > > >
> > > > 1) How do we share clk details on instantiated device to find is it
> > > > connected to external crystal or external clock source? as we cannot
> > > > pass of_node between PMIC and "i2c_board_info" as it results in
> > > > pinctrl failure. info->platformdata and
> > > > Client->dev.platformdata to retrieve this info??
> > >
> > > Or
> > >
> > > I2C instantiation based on actual oscillator bit value, ie, two
> > > i2c_device_id's with one for setting oscillator bit and another for
> > > clearing oscillator bit
> > >
> > > PMIC driver parses the clock details. Based on firmware version and
> > > clock, It instantiates either i2c_device_id with setting oscillator
> > > bit or clearing oscillator bit.
> >
> > I don't like that hack. I still think that two DT nodes is the best
> > option, I think you're trying hard to hack around a problem that is
> > actually not a problem.
>
> Why do you think it is a hack? I believe rather it is actual solution
>
> PMIC is a single device, with 2 regs, clocks, pinctrl and IRQ properties.
> So it will be represented as single node with single compatible.
The chip is a single package that contains two independent devices. This
is not different than bundling many IP cores in an SoC, we have one DT
node per IP core, not a single DT node for the SoC. The fact that we're
dealing with an external physical component here isn't relevant.
> By instating a client device, we are sharing the relevant resources to
> RTC device driver.
By instantiating a client device, you create a second struct device,
which is the kernel abstraction of a hardware device. This shows in my
opinion that we're dealing with two devices here, hence my
recommendation of using two DT nodes.
As you've noticed, with two devices and a single DT node, pinctrl
complains. You can hack around that by moving the pinctrl configuration
from the PMIC DT node to another DT node, and that's one first hack.
Then, you'll need to have two different device IDs depending on the PMIC
version to let the RTC driver set the oscillator bit correctly, and
that's a second hack.
A solution with two DT nodes models the hardware better and is cleaner.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH v4] mtd: rawnand: macronix: OTP access for MX30LFxG18AC
From: Arseniy Krasnov @ 2023-06-14 9:24 UTC (permalink / raw)
To: Miquel Raynal, liao jaime
Cc: Richard Weinberger, Vignesh Raghavendra, Sumit Semwal,
Christian König, oxffffaa, kernel, Boris Brezillon,
Jaime Liao, Mason Yang, linux-mtd, linux-kernel, linux-media,
dri-devel, linaro-mm-sig
In-Reply-To: <24dcecf2-f394-6542-eeb6-ab65ea19708a@sberdevices.ru>
On 14.06.2023 12:22, Arseniy Krasnov wrote:
> Hello Miquel and Jaime!
>
> On 14.06.2023 12:10, Miquel Raynal wrote:
>> Hi liao,
>>
>> jaimeliao.tw@gmail.com wrote on Wed, 14 Jun 2023 17:06:16 +0800:
>>
>>> Hi Miquel
>>>
>>>
>>>>
>>>> Hello,
>>>>
>>>> AVKrasnov@sberdevices.ru wrote on Tue, 23 May 2023 13:16:34 +0300:
>>>>
>>>>> This adds support for OTP area access on MX30LFxG18AC chip series.
>>>>
>>>> Jaime, any feedback on this? Will you test it?
>>>>
>>>> How are we supposed to test the OTP is locked? I see this is still an
>>>> open point.
>>> After checking with internal, sub feature parameter are volatile register.
>>>
>>> It could be change after enter/exit OTP region or power cycle even OTP
>>>
>>> region have been locked.
>>>
>>> OTP operation mode still could be enter/exit and region is read only
>>> after OTP in protect mode.
>>>
>>> #program command could execute but no use after setting OTP region in
>>> protect mode.
>>>
>>> So that we can't check whether OTP region is locked via get feature.
>>>
>>> And we don't have region for checking status of OTP locked.
>>
>> Ah, too bad. But thanks a lot for the explanation. Arseniy, can you
>> please change your comment to explain that the bit is volatile and thus
>> there is no way to check if an otp region is locked? I would return
>> EOPNOTSUPP in this case and verify that the core cleanly handles the
>> situation.
>
> Ok, thanks for details. @Miquel, ok, I'll change comment from "don't know..."
> to this explanation. About EOPNOTSUPP, IIUC I think it is not good way to
> suppress '_get_fact_prot_info' and '_get_user_prot_info' callbacks with this
> return code as it is the only way to know size of OTP region.
>
> Thanks, Arseniy
Ahh sorry, there is '_lock_user_prot_reg', it will return EOPNOTSUPP. Now I see:)
Thanks, Arseniy
>
>>
>> Thanks,
>> Miquèl
>>
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>>>>> ---
>>>>> v1 -> v2:
>>>>> * Add slab.h include due to kernel test robot error.
>>>>> v2 -> v3:
>>>>> * Use 'uint64_t' as input argument for 'do_div()' instead
>>>>> of 'unsigned long' due to kernel test robot error.
>>>>> v3 -> v4:
>>>>> * Use 'dev_err()' instead of 'WARN()'.
>>>>> * Call 'match_string()' before checking 'supports_set_get_features'
>>>>> in 'macronix_nand_setup_otp().
>>>>> * Use 'u8' instead of 'uint8_t' as ./checkpatch.pl wants.
>>>>>
>>>>> drivers/mtd/nand/raw/nand_macronix.c | 216 +++++++++++++++++++++++++++
>>>>> 1 file changed, 216 insertions(+)
>>>>>
>>>>> diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c
>>>>> index 1472f925f386..be1ffa93bebb 100644
>>>>> --- a/drivers/mtd/nand/raw/nand_macronix.c
>>>>> +++ b/drivers/mtd/nand/raw/nand_macronix.c
>>>>> @@ -6,6 +6,7 @@
>>>>> * Author: Boris Brezillon <boris.brezillon@free-electrons.com>
>>>>> */
>>>>>
>>>>> +#include <linux/slab.h>
>>>>> #include "linux/delay.h"
>>>>> #include "internals.h"
>>>>>
>>>>> @@ -31,6 +32,20 @@
>>>>>
>>>>> #define MXIC_CMD_POWER_DOWN 0xB9
>>>>>
>>>>> +#define ONFI_FEATURE_ADDR_30LFXG18AC_OTP 0x90
>>>>> +#define MACRONIX_30LFXG18AC_OTP_START_PAGE 0
>>>>> +#define MACRONIX_30LFXG18AC_OTP_PAGES 30
>>>>> +#define MACRONIX_30LFXG18AC_OTP_PAGE_SIZE 2112
>>>>> +#define MACRONIX_30LFXG18AC_OTP_START_BYTE \
>>>>> + (MACRONIX_30LFXG18AC_OTP_START_PAGE * \
>>>>> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
>>>>> +#define MACRONIX_30LFXG18AC_OTP_SIZE_BYTES \
>>>>> + (MACRONIX_30LFXG18AC_OTP_PAGES * \
>>>>> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
>>>>> +
>>>>> +#define MACRONIX_30LFXG18AC_OTP_EN BIT(0)
>>>>> +#define MACRONIX_30LFXG18AC_OTP_LOCKED BIT(1)
>>>>> +
>>>>> struct nand_onfi_vendor_macronix {
>>>>> u8 reserved;
>>>>> u8 reliability_func;
>>>>> @@ -316,6 +331,206 @@ static void macronix_nand_deep_power_down_support(struct nand_chip *chip)
>>>>> chip->ops.resume = mxic_nand_resume;
>>>>> }
>>>>>
>>>>> +static int macronix_30lfxg18ac_get_otp_info(struct mtd_info *mtd, size_t len,
>>>>> + size_t *retlen,
>>>>> + struct otp_info *buf)
>>>>> +{
>>>>> + if (len < sizeof(*buf))
>>>>> + return -EINVAL;
>>>>> +
>>>>> + /* Don't know how to check that OTP is locked. */
>>>>> + buf->locked = 0;
>>>>> + buf->start = MACRONIX_30LFXG18AC_OTP_START_BYTE;
>>>>> + buf->length = MACRONIX_30LFXG18AC_OTP_SIZE_BYTES;
>>>>> +
>>>>> + *retlen = sizeof(*buf);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int macronix_30lfxg18ac_otp_enable(struct nand_chip *nand)
>>>>> +{
>>>>> + u8 feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
>>>>> +
>>>>> + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN;
>>>>> + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
>>>>> + feature_buf);
>>>>> +}
>>>>> +
>>>>> +static int macronix_30lfxg18ac_otp_disable(struct nand_chip *nand)
>>>>> +{
>>>>> + u8 feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
>>>>> +
>>>>> + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
>>>>> + feature_buf);
>>>>> +}
>>>>> +
>>>>> +static int __macronix_30lfxg18ac_rw_otp(struct mtd_info *mtd,
>>>>> + loff_t offs_in_flash,
>>>>> + size_t len, size_t *retlen,
>>>>> + u_char *buf, bool write)
>>>>> +{
>>>>> + struct nand_chip *nand;
>>>>> + size_t bytes_handled;
>>>>> + off_t offs_in_page;
>>>>> + void *dma_buf;
>>>>> + u64 page;
>>>>> + int ret;
>>>>> +
>>>>> + /* 'nand_prog/read_page_op()' may use 'buf' as DMA buffer,
>>>>> + * so allocate properly aligned memory for it. This is
>>>>> + * needed because cross page accesses may lead to unaligned
>>>>> + * buffer address for DMA.
>>>>> + */
>>>>> + dma_buf = kmalloc(MACRONIX_30LFXG18AC_OTP_PAGE_SIZE, GFP_KERNEL);
>>>>> + if (!dma_buf)
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + nand = mtd_to_nand(mtd);
>>>>> + nand_select_target(nand, 0);
>>>>> +
>>>>> + ret = macronix_30lfxg18ac_otp_enable(nand);
>>>>> + if (ret)
>>>>> + goto out_otp;
>>>>> +
>>>>> + page = offs_in_flash;
>>>>> + /* 'page' will be result of division. */
>>>>> + offs_in_page = do_div(page, MACRONIX_30LFXG18AC_OTP_PAGE_SIZE);
>>>>> + bytes_handled = 0;
>>>>> +
>>>>> + while (bytes_handled < len &&
>>>>> + page < MACRONIX_30LFXG18AC_OTP_PAGES) {
>>>>> + size_t bytes_to_handle;
>>>>> +
>>>>> + bytes_to_handle = min_t(size_t, len - bytes_handled,
>>>>> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE -
>>>>> + offs_in_page);
>>>>> +
>>>>> + if (write) {
>>>>> + memcpy(dma_buf, &buf[bytes_handled], bytes_to_handle);
>>>>> + ret = nand_prog_page_op(nand, page, offs_in_page,
>>>>> + dma_buf, bytes_to_handle);
>>>>> + } else {
>>>>> + ret = nand_read_page_op(nand, page, offs_in_page,
>>>>> + dma_buf, bytes_to_handle);
>>>>> + if (!ret)
>>>>> + memcpy(&buf[bytes_handled], dma_buf,
>>>>> + bytes_to_handle);
>>>>> + }
>>>>> + if (ret)
>>>>> + goto out_otp;
>>>>> +
>>>>> + bytes_handled += bytes_to_handle;
>>>>> + offs_in_page = 0;
>>>>> + page++;
>>>>> + }
>>>>> +
>>>>> + *retlen = bytes_handled;
>>>>> +
>>>>> +out_otp:
>>>>> + if (ret)
>>>>> + dev_err(&mtd->dev, "failed to perform OTP IO: %i\n", ret);
>>>>> +
>>>>> + ret = macronix_30lfxg18ac_otp_disable(nand);
>>>>> + if (ret)
>>>>> + dev_err(&mtd->dev, "failed to leave OTP mode after %s\n",
>>>>> + write ? "write" : "read");
>>>>> +
>>>>> + nand_deselect_target(nand);
>>>>> + kfree(dma_buf);
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +static int macronix_30lfxg18ac_write_otp(struct mtd_info *mtd, loff_t to,
>>>>> + size_t len, size_t *rlen,
>>>>> + const u_char *buf)
>>>>> +{
>>>>> + return __macronix_30lfxg18ac_rw_otp(mtd, to, len, rlen, (u_char *)buf,
>>>>> + true);
>>>>> +}
>>>>> +
>>>>> +static int macronix_30lfxg18ac_read_otp(struct mtd_info *mtd, loff_t from,
>>>>> + size_t len, size_t *rlen,
>>>>> + u_char *buf)
>>>>> +{
>>>>> + return __macronix_30lfxg18ac_rw_otp(mtd, from, len, rlen, buf, false);
>>>>> +}
>>>>> +
>>>>> +static int macronix_30lfxg18ac_lock_otp(struct mtd_info *mtd, loff_t from,
>>>>> + size_t len)
>>>>> +{
>>>>> + u8 feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
>>>>> + struct nand_chip *nand;
>>>>> + int ret;
>>>>> +
>>>>> + if (from != MACRONIX_30LFXG18AC_OTP_START_BYTE ||
>>>>> + len != MACRONIX_30LFXG18AC_OTP_SIZE_BYTES)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + dev_dbg(&mtd->dev, "locking OTP\n");
>>>>> +
>>>>> + nand = mtd_to_nand(mtd);
>>>>> + nand_select_target(nand, 0);
>>>>> +
>>>>> + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN |
>>>>> + MACRONIX_30LFXG18AC_OTP_LOCKED;
>>>>> + ret = nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
>>>>> + feature_buf);
>>>>> + if (ret) {
>>>>> + dev_err(&mtd->dev,
>>>>> + "failed to lock OTP (set features): %i\n", ret);
>>>>> + nand_deselect_target(nand);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + /* Do dummy page prog with zero address. */
>>>>> + feature_buf[0] = 0;
>>>>> + ret = nand_prog_page_op(nand, 0, 0, feature_buf, 1);
>>>>> + if (ret)
>>>>> + dev_err(&mtd->dev,
>>>>> + "failed to lock OTP (page prog): %i\n", ret);
>>>>> +
>>>>> + ret = macronix_30lfxg18ac_otp_disable(nand);
>>>>> + if (ret)
>>>>> + dev_err(&mtd->dev, "failed to leave OTP mode after lock\n");
>>>>> +
>>>>> + nand_deselect_target(nand);
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +static void macronix_nand_setup_otp(struct nand_chip *chip)
>>>>> +{
>>>>> + static const char * const supported_otp_models[] = {
>>>>> + "MX30LF1G18AC",
>>>>> + "MX30LF2G18AC",
>>>>> + "MX30LF4G18AC",
>>>>> + };
>>>>> + struct mtd_info *mtd;
>>>>> +
>>>>> + if (match_string(supported_otp_models,
>>>>> + ARRAY_SIZE(supported_otp_models),
>>>>> + chip->parameters.model) < 0)
>>>>> + return;
>>>>> +
>>>>> + if (!chip->parameters.supports_set_get_features)
>>>>> + return;
>>>>> +
>>>>> + bitmap_set(chip->parameters.get_feature_list,
>>>>> + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
>>>>> + bitmap_set(chip->parameters.set_feature_list,
>>>>> + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
>>>>> +
>>>>> + mtd = nand_to_mtd(chip);
>>>>> + mtd->_get_fact_prot_info = macronix_30lfxg18ac_get_otp_info;
>>>>> + mtd->_read_fact_prot_reg = macronix_30lfxg18ac_read_otp;
>>>>> + mtd->_get_user_prot_info = macronix_30lfxg18ac_get_otp_info;
>>>>> + mtd->_read_user_prot_reg = macronix_30lfxg18ac_read_otp;
>>>>> + mtd->_write_user_prot_reg = macronix_30lfxg18ac_write_otp;
>>>>> + mtd->_lock_user_prot_reg = macronix_30lfxg18ac_lock_otp;
>>>>> +}
>>>>> +
>>>>> static int macronix_nand_init(struct nand_chip *chip)
>>>>> {
>>>>> if (nand_is_slc(chip))
>>>>> @@ -325,6 +540,7 @@ static int macronix_nand_init(struct nand_chip *chip)
>>>>> macronix_nand_onfi_init(chip);
>>>>> macronix_nand_block_protection_support(chip);
>>>>> macronix_nand_deep_power_down_support(chip);
>>>>> + macronix_nand_setup_otp(chip);
>>>>>
>>>>> return 0;
>>>>> }
>>>>
>>>>
>>>> Thanks,
>>>> Miquèl
>>>
>>> Thanks
>>> Jaime
>>
>>
^ permalink raw reply
* Re: [PATCH v4] mtd: rawnand: macronix: OTP access for MX30LFxG18AC
From: Arseniy Krasnov @ 2023-06-14 9:22 UTC (permalink / raw)
To: Miquel Raynal, liao jaime
Cc: Richard Weinberger, Vignesh Raghavendra, Sumit Semwal,
Christian König, oxffffaa, kernel, Boris Brezillon,
Jaime Liao, Mason Yang, linux-mtd, linux-kernel, linux-media,
dri-devel, linaro-mm-sig
In-Reply-To: <20230614111001.6b0417d4@xps-13>
Hello Miquel and Jaime!
On 14.06.2023 12:10, Miquel Raynal wrote:
> Hi liao,
>
> jaimeliao.tw@gmail.com wrote on Wed, 14 Jun 2023 17:06:16 +0800:
>
>> Hi Miquel
>>
>>
>>>
>>> Hello,
>>>
>>> AVKrasnov@sberdevices.ru wrote on Tue, 23 May 2023 13:16:34 +0300:
>>>
>>>> This adds support for OTP area access on MX30LFxG18AC chip series.
>>>
>>> Jaime, any feedback on this? Will you test it?
>>>
>>> How are we supposed to test the OTP is locked? I see this is still an
>>> open point.
>> After checking with internal, sub feature parameter are volatile register.
>>
>> It could be change after enter/exit OTP region or power cycle even OTP
>>
>> region have been locked.
>>
>> OTP operation mode still could be enter/exit and region is read only
>> after OTP in protect mode.
>>
>> #program command could execute but no use after setting OTP region in
>> protect mode.
>>
>> So that we can't check whether OTP region is locked via get feature.
>>
>> And we don't have region for checking status of OTP locked.
>
> Ah, too bad. But thanks a lot for the explanation. Arseniy, can you
> please change your comment to explain that the bit is volatile and thus
> there is no way to check if an otp region is locked? I would return
> EOPNOTSUPP in this case and verify that the core cleanly handles the
> situation.
Ok, thanks for details. @Miquel, ok, I'll change comment from "don't know..."
to this explanation. About EOPNOTSUPP, IIUC I think it is not good way to
suppress '_get_fact_prot_info' and '_get_user_prot_info' callbacks with this
return code as it is the only way to know size of OTP region.
Thanks, Arseniy
>
> Thanks,
> Miquèl
>
>>
>>>
>>>>
>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>>>> ---
>>>> v1 -> v2:
>>>> * Add slab.h include due to kernel test robot error.
>>>> v2 -> v3:
>>>> * Use 'uint64_t' as input argument for 'do_div()' instead
>>>> of 'unsigned long' due to kernel test robot error.
>>>> v3 -> v4:
>>>> * Use 'dev_err()' instead of 'WARN()'.
>>>> * Call 'match_string()' before checking 'supports_set_get_features'
>>>> in 'macronix_nand_setup_otp().
>>>> * Use 'u8' instead of 'uint8_t' as ./checkpatch.pl wants.
>>>>
>>>> drivers/mtd/nand/raw/nand_macronix.c | 216 +++++++++++++++++++++++++++
>>>> 1 file changed, 216 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c
>>>> index 1472f925f386..be1ffa93bebb 100644
>>>> --- a/drivers/mtd/nand/raw/nand_macronix.c
>>>> +++ b/drivers/mtd/nand/raw/nand_macronix.c
>>>> @@ -6,6 +6,7 @@
>>>> * Author: Boris Brezillon <boris.brezillon@free-electrons.com>
>>>> */
>>>>
>>>> +#include <linux/slab.h>
>>>> #include "linux/delay.h"
>>>> #include "internals.h"
>>>>
>>>> @@ -31,6 +32,20 @@
>>>>
>>>> #define MXIC_CMD_POWER_DOWN 0xB9
>>>>
>>>> +#define ONFI_FEATURE_ADDR_30LFXG18AC_OTP 0x90
>>>> +#define MACRONIX_30LFXG18AC_OTP_START_PAGE 0
>>>> +#define MACRONIX_30LFXG18AC_OTP_PAGES 30
>>>> +#define MACRONIX_30LFXG18AC_OTP_PAGE_SIZE 2112
>>>> +#define MACRONIX_30LFXG18AC_OTP_START_BYTE \
>>>> + (MACRONIX_30LFXG18AC_OTP_START_PAGE * \
>>>> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
>>>> +#define MACRONIX_30LFXG18AC_OTP_SIZE_BYTES \
>>>> + (MACRONIX_30LFXG18AC_OTP_PAGES * \
>>>> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
>>>> +
>>>> +#define MACRONIX_30LFXG18AC_OTP_EN BIT(0)
>>>> +#define MACRONIX_30LFXG18AC_OTP_LOCKED BIT(1)
>>>> +
>>>> struct nand_onfi_vendor_macronix {
>>>> u8 reserved;
>>>> u8 reliability_func;
>>>> @@ -316,6 +331,206 @@ static void macronix_nand_deep_power_down_support(struct nand_chip *chip)
>>>> chip->ops.resume = mxic_nand_resume;
>>>> }
>>>>
>>>> +static int macronix_30lfxg18ac_get_otp_info(struct mtd_info *mtd, size_t len,
>>>> + size_t *retlen,
>>>> + struct otp_info *buf)
>>>> +{
>>>> + if (len < sizeof(*buf))
>>>> + return -EINVAL;
>>>> +
>>>> + /* Don't know how to check that OTP is locked. */
>>>> + buf->locked = 0;
>>>> + buf->start = MACRONIX_30LFXG18AC_OTP_START_BYTE;
>>>> + buf->length = MACRONIX_30LFXG18AC_OTP_SIZE_BYTES;
>>>> +
>>>> + *retlen = sizeof(*buf);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int macronix_30lfxg18ac_otp_enable(struct nand_chip *nand)
>>>> +{
>>>> + u8 feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
>>>> +
>>>> + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN;
>>>> + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
>>>> + feature_buf);
>>>> +}
>>>> +
>>>> +static int macronix_30lfxg18ac_otp_disable(struct nand_chip *nand)
>>>> +{
>>>> + u8 feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
>>>> +
>>>> + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
>>>> + feature_buf);
>>>> +}
>>>> +
>>>> +static int __macronix_30lfxg18ac_rw_otp(struct mtd_info *mtd,
>>>> + loff_t offs_in_flash,
>>>> + size_t len, size_t *retlen,
>>>> + u_char *buf, bool write)
>>>> +{
>>>> + struct nand_chip *nand;
>>>> + size_t bytes_handled;
>>>> + off_t offs_in_page;
>>>> + void *dma_buf;
>>>> + u64 page;
>>>> + int ret;
>>>> +
>>>> + /* 'nand_prog/read_page_op()' may use 'buf' as DMA buffer,
>>>> + * so allocate properly aligned memory for it. This is
>>>> + * needed because cross page accesses may lead to unaligned
>>>> + * buffer address for DMA.
>>>> + */
>>>> + dma_buf = kmalloc(MACRONIX_30LFXG18AC_OTP_PAGE_SIZE, GFP_KERNEL);
>>>> + if (!dma_buf)
>>>> + return -ENOMEM;
>>>> +
>>>> + nand = mtd_to_nand(mtd);
>>>> + nand_select_target(nand, 0);
>>>> +
>>>> + ret = macronix_30lfxg18ac_otp_enable(nand);
>>>> + if (ret)
>>>> + goto out_otp;
>>>> +
>>>> + page = offs_in_flash;
>>>> + /* 'page' will be result of division. */
>>>> + offs_in_page = do_div(page, MACRONIX_30LFXG18AC_OTP_PAGE_SIZE);
>>>> + bytes_handled = 0;
>>>> +
>>>> + while (bytes_handled < len &&
>>>> + page < MACRONIX_30LFXG18AC_OTP_PAGES) {
>>>> + size_t bytes_to_handle;
>>>> +
>>>> + bytes_to_handle = min_t(size_t, len - bytes_handled,
>>>> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE -
>>>> + offs_in_page);
>>>> +
>>>> + if (write) {
>>>> + memcpy(dma_buf, &buf[bytes_handled], bytes_to_handle);
>>>> + ret = nand_prog_page_op(nand, page, offs_in_page,
>>>> + dma_buf, bytes_to_handle);
>>>> + } else {
>>>> + ret = nand_read_page_op(nand, page, offs_in_page,
>>>> + dma_buf, bytes_to_handle);
>>>> + if (!ret)
>>>> + memcpy(&buf[bytes_handled], dma_buf,
>>>> + bytes_to_handle);
>>>> + }
>>>> + if (ret)
>>>> + goto out_otp;
>>>> +
>>>> + bytes_handled += bytes_to_handle;
>>>> + offs_in_page = 0;
>>>> + page++;
>>>> + }
>>>> +
>>>> + *retlen = bytes_handled;
>>>> +
>>>> +out_otp:
>>>> + if (ret)
>>>> + dev_err(&mtd->dev, "failed to perform OTP IO: %i\n", ret);
>>>> +
>>>> + ret = macronix_30lfxg18ac_otp_disable(nand);
>>>> + if (ret)
>>>> + dev_err(&mtd->dev, "failed to leave OTP mode after %s\n",
>>>> + write ? "write" : "read");
>>>> +
>>>> + nand_deselect_target(nand);
>>>> + kfree(dma_buf);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int macronix_30lfxg18ac_write_otp(struct mtd_info *mtd, loff_t to,
>>>> + size_t len, size_t *rlen,
>>>> + const u_char *buf)
>>>> +{
>>>> + return __macronix_30lfxg18ac_rw_otp(mtd, to, len, rlen, (u_char *)buf,
>>>> + true);
>>>> +}
>>>> +
>>>> +static int macronix_30lfxg18ac_read_otp(struct mtd_info *mtd, loff_t from,
>>>> + size_t len, size_t *rlen,
>>>> + u_char *buf)
>>>> +{
>>>> + return __macronix_30lfxg18ac_rw_otp(mtd, from, len, rlen, buf, false);
>>>> +}
>>>> +
>>>> +static int macronix_30lfxg18ac_lock_otp(struct mtd_info *mtd, loff_t from,
>>>> + size_t len)
>>>> +{
>>>> + u8 feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
>>>> + struct nand_chip *nand;
>>>> + int ret;
>>>> +
>>>> + if (from != MACRONIX_30LFXG18AC_OTP_START_BYTE ||
>>>> + len != MACRONIX_30LFXG18AC_OTP_SIZE_BYTES)
>>>> + return -EINVAL;
>>>> +
>>>> + dev_dbg(&mtd->dev, "locking OTP\n");
>>>> +
>>>> + nand = mtd_to_nand(mtd);
>>>> + nand_select_target(nand, 0);
>>>> +
>>>> + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN |
>>>> + MACRONIX_30LFXG18AC_OTP_LOCKED;
>>>> + ret = nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
>>>> + feature_buf);
>>>> + if (ret) {
>>>> + dev_err(&mtd->dev,
>>>> + "failed to lock OTP (set features): %i\n", ret);
>>>> + nand_deselect_target(nand);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + /* Do dummy page prog with zero address. */
>>>> + feature_buf[0] = 0;
>>>> + ret = nand_prog_page_op(nand, 0, 0, feature_buf, 1);
>>>> + if (ret)
>>>> + dev_err(&mtd->dev,
>>>> + "failed to lock OTP (page prog): %i\n", ret);
>>>> +
>>>> + ret = macronix_30lfxg18ac_otp_disable(nand);
>>>> + if (ret)
>>>> + dev_err(&mtd->dev, "failed to leave OTP mode after lock\n");
>>>> +
>>>> + nand_deselect_target(nand);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static void macronix_nand_setup_otp(struct nand_chip *chip)
>>>> +{
>>>> + static const char * const supported_otp_models[] = {
>>>> + "MX30LF1G18AC",
>>>> + "MX30LF2G18AC",
>>>> + "MX30LF4G18AC",
>>>> + };
>>>> + struct mtd_info *mtd;
>>>> +
>>>> + if (match_string(supported_otp_models,
>>>> + ARRAY_SIZE(supported_otp_models),
>>>> + chip->parameters.model) < 0)
>>>> + return;
>>>> +
>>>> + if (!chip->parameters.supports_set_get_features)
>>>> + return;
>>>> +
>>>> + bitmap_set(chip->parameters.get_feature_list,
>>>> + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
>>>> + bitmap_set(chip->parameters.set_feature_list,
>>>> + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
>>>> +
>>>> + mtd = nand_to_mtd(chip);
>>>> + mtd->_get_fact_prot_info = macronix_30lfxg18ac_get_otp_info;
>>>> + mtd->_read_fact_prot_reg = macronix_30lfxg18ac_read_otp;
>>>> + mtd->_get_user_prot_info = macronix_30lfxg18ac_get_otp_info;
>>>> + mtd->_read_user_prot_reg = macronix_30lfxg18ac_read_otp;
>>>> + mtd->_write_user_prot_reg = macronix_30lfxg18ac_write_otp;
>>>> + mtd->_lock_user_prot_reg = macronix_30lfxg18ac_lock_otp;
>>>> +}
>>>> +
>>>> static int macronix_nand_init(struct nand_chip *chip)
>>>> {
>>>> if (nand_is_slc(chip))
>>>> @@ -325,6 +540,7 @@ static int macronix_nand_init(struct nand_chip *chip)
>>>> macronix_nand_onfi_init(chip);
>>>> macronix_nand_block_protection_support(chip);
>>>> macronix_nand_deep_power_down_support(chip);
>>>> + macronix_nand_setup_otp(chip);
>>>>
>>>> return 0;
>>>> }
>>>
>>>
>>> Thanks,
>>> Miquèl
>>
>> Thanks
>> Jaime
>
>
^ permalink raw reply
* Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
From: Geert Uytterhoeven @ 2023-06-14 9:18 UTC (permalink / raw)
To: Biju Das
Cc: Laurent Pinchart, Wolfram Sang, Alexandre Belloni, Mark Brown,
Krzysztof Kozlowski, Rob Herring, Conor Dooley, Andrzej Hajda,
Neil Armstrong, Robert Foss, David Airlie, Daniel Vetter,
Kieran Bingham, Mauro Carvalho Chehab, Hans Verkuil,
Alessandro Zummo, Jonas Karlman, Jernej Skrabec,
Uwe Kleine-König, Corey Minyard, Marek Behún,
Jiasheng Jiang, Antonio Borneo, Abhinav Kumar, Ahmad Fatoum,
dri-devel@lists.freedesktop.org, linux-i2c@vger.kernel.org,
linux-media@vger.kernel.org, Fabrizio Castro,
linux-renesas-soc@vger.kernel.org
In-Reply-To: <OS0PR01MB59225C45554667D342454923865AA@OS0PR01MB5922.jpnprd01.prod.outlook.com>
Hi Biju,
On Wed, Jun 14, 2023 at 10:21 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
> > On Tue, Jun 13, 2023 at 07:31:46PM +0000, Biju Das wrote:
> > > > Subject: RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device
> > > > API
> > > > > Subject: RE: [PATCH v5 01/11] i2c: Enhance
> > > > > i2c_new_ancillary_device API
> > > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance
> > > > > > i2c_new_ancillary_device API
> > > > > >
> > > > > > Hi everyone,
> > > > > >
> > > > > > > Perhaps we should first think through what an ancillary device
> > > > > > > really is. My understanding is that it is used to talk to
> > > > > > > secondary addresses of a multi-address I2C slave device.
> > > > > >
> > > > > > As I mentioned somewhere before, this is not the case. Ancillary
> > > > > > devices are when one *driver* handles more than one address.
> > > > > > Everything else has been handled differently in the past (for
> > > > > > all the uses I am aware of).
> > > > > >
> > > > > > Yet, I have another idea which is so simple that I wonder if it
> > > > > > maybe has already been discussed so far?
> > > > > >
> > > > > > * have two regs in the bindings
> > > > >
> > > > > OK, it is inline with DT maintainers expectation as it is matching
> > > > > with real hw as single device node having two regs.
> > > > >
> > > > > > * use the second reg with i2c_new_client_device to instantiate the
> > > > > > RTC sibling. 'struct i2c_board_info', which is one parameter,
> > should
> > > > > > have enough options to pass data, e.g it has a software_node.
> > > > >
> > > > > OK, I can see the below can be passed from PMIC to new client
> > device.
> > > > >
> > > > > client->addr = info->addr;
> > > > >
> > > > > client->init_irq = info->irq;
> > > > >
> > > > > >
> > > > > > Should work or did I miss something here?
> > > > >
> > > > > I guess it will work. We instantiate appropriate device based On
> > > > > PMIC revision and slave address and IRQ resource passed through
> > > > > 'struct i2c_board_info'
> > > > >
> > > > > Will check this and update you.
> > > >
> > > > info.irq = irq; -->Irq fine
> > > > info.addr = addr; -->slave address fine size = strscpy(info.type,
> > > > name, sizeof(info.type)); -->instantiation based on PMIC version
> > > > fine.
> > > >
> > > > 1) How do we share clk details on instantiated device to find is it
> > > > connected to external crystal or external clock source? as we cannot
> > > > pass of_node between PMIC and "i2c_board_info" as it results in
> > > > pinctrl failure. info->platformdata and
> > > > Client->dev.platformdata to retrieve this info??
> > >
> > > Or
> > >
> > > I2C instantiation based on actual oscillator bit value, ie, two
> > > i2c_device_id's with one for setting oscillator bit and another for
> > > clearing oscillator bit
> > >
> > > PMIC driver parses the clock details. Based on firmware version and
> > > clock, It instantiates either i2c_device_id with setting oscillator
> > > bit or clearing oscillator bit.
> >
> > I don't like that hack. I still think that two DT nodes is the best
> > option, I think you're trying hard to hack around a problem that is
> > actually not a problem.
>
> Why do you think it is a hack? I believe rather it is actual solution
>
> PMIC is a single device, with 2 regs, clocks, pinctrl and IRQ properties.
> So it will be represented as single node with single compatible.
>
> By instating a client device, we are sharing the relevant resources to RTC device driver.
Exactly. RAA215300 is a PMIC with an integrated ISL1208-derivative.
My biggest concern with using 2 separate nodes in DT is that one day
we might discover another integration issue, which needs communication
between the two parts.
Things from the top of my head:
1. The device has a single interrupt pin. Is there any interaction
or coordination between PMIC and RTC interrupts?
2. On the real ISL1208, the interrupt pin can also be used as a clock
output. Perhaps this is fed to some PMIC part in the
RAA215300, too?
2. Does the battery charger circuit in the PMIC impact the VBAT
input of the RTC?
3. Are there other I2C addresses the chip listens to?
I only have access to the Short-Form Datasheet for the RAA215300,
so I cannot check myself...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v4] mtd: rawnand: macronix: OTP access for MX30LFxG18AC
From: Miquel Raynal @ 2023-06-14 9:10 UTC (permalink / raw)
To: liao jaime
Cc: Arseniy Krasnov, Richard Weinberger, Vignesh Raghavendra,
Sumit Semwal, Christian König, oxffffaa, kernel,
Boris Brezillon, Jaime Liao, Mason Yang, linux-mtd, linux-kernel,
linux-media, dri-devel, linaro-mm-sig
In-Reply-To: <CAAQoYR=aU-tpFYhfKUae=2zbvpzmP3_d4PYp_252qxSsPcVbaQ@mail.gmail.com>
Hi liao,
jaimeliao.tw@gmail.com wrote on Wed, 14 Jun 2023 17:06:16 +0800:
> Hi Miquel
>
>
> >
> > Hello,
> >
> > AVKrasnov@sberdevices.ru wrote on Tue, 23 May 2023 13:16:34 +0300:
> >
> > > This adds support for OTP area access on MX30LFxG18AC chip series.
> >
> > Jaime, any feedback on this? Will you test it?
> >
> > How are we supposed to test the OTP is locked? I see this is still an
> > open point.
> After checking with internal, sub feature parameter are volatile register.
>
> It could be change after enter/exit OTP region or power cycle even OTP
>
> region have been locked.
>
> OTP operation mode still could be enter/exit and region is read only
> after OTP in protect mode.
>
> #program command could execute but no use after setting OTP region in
> protect mode.
>
> So that we can't check whether OTP region is locked via get feature.
>
> And we don't have region for checking status of OTP locked.
Ah, too bad. But thanks a lot for the explanation. Arseniy, can you
please change your comment to explain that the bit is volatile and thus
there is no way to check if an otp region is locked? I would return
EOPNOTSUPP in this case and verify that the core cleanly handles the
situation.
Thanks,
Miquèl
>
> >
> > >
> > > Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> > > ---
> > > v1 -> v2:
> > > * Add slab.h include due to kernel test robot error.
> > > v2 -> v3:
> > > * Use 'uint64_t' as input argument for 'do_div()' instead
> > > of 'unsigned long' due to kernel test robot error.
> > > v3 -> v4:
> > > * Use 'dev_err()' instead of 'WARN()'.
> > > * Call 'match_string()' before checking 'supports_set_get_features'
> > > in 'macronix_nand_setup_otp().
> > > * Use 'u8' instead of 'uint8_t' as ./checkpatch.pl wants.
> > >
> > > drivers/mtd/nand/raw/nand_macronix.c | 216 +++++++++++++++++++++++++++
> > > 1 file changed, 216 insertions(+)
> > >
> > > diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c
> > > index 1472f925f386..be1ffa93bebb 100644
> > > --- a/drivers/mtd/nand/raw/nand_macronix.c
> > > +++ b/drivers/mtd/nand/raw/nand_macronix.c
> > > @@ -6,6 +6,7 @@
> > > * Author: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > */
> > >
> > > +#include <linux/slab.h>
> > > #include "linux/delay.h"
> > > #include "internals.h"
> > >
> > > @@ -31,6 +32,20 @@
> > >
> > > #define MXIC_CMD_POWER_DOWN 0xB9
> > >
> > > +#define ONFI_FEATURE_ADDR_30LFXG18AC_OTP 0x90
> > > +#define MACRONIX_30LFXG18AC_OTP_START_PAGE 0
> > > +#define MACRONIX_30LFXG18AC_OTP_PAGES 30
> > > +#define MACRONIX_30LFXG18AC_OTP_PAGE_SIZE 2112
> > > +#define MACRONIX_30LFXG18AC_OTP_START_BYTE \
> > > + (MACRONIX_30LFXG18AC_OTP_START_PAGE * \
> > > + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
> > > +#define MACRONIX_30LFXG18AC_OTP_SIZE_BYTES \
> > > + (MACRONIX_30LFXG18AC_OTP_PAGES * \
> > > + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
> > > +
> > > +#define MACRONIX_30LFXG18AC_OTP_EN BIT(0)
> > > +#define MACRONIX_30LFXG18AC_OTP_LOCKED BIT(1)
> > > +
> > > struct nand_onfi_vendor_macronix {
> > > u8 reserved;
> > > u8 reliability_func;
> > > @@ -316,6 +331,206 @@ static void macronix_nand_deep_power_down_support(struct nand_chip *chip)
> > > chip->ops.resume = mxic_nand_resume;
> > > }
> > >
> > > +static int macronix_30lfxg18ac_get_otp_info(struct mtd_info *mtd, size_t len,
> > > + size_t *retlen,
> > > + struct otp_info *buf)
> > > +{
> > > + if (len < sizeof(*buf))
> > > + return -EINVAL;
> > > +
> > > + /* Don't know how to check that OTP is locked. */
> > > + buf->locked = 0;
> > > + buf->start = MACRONIX_30LFXG18AC_OTP_START_BYTE;
> > > + buf->length = MACRONIX_30LFXG18AC_OTP_SIZE_BYTES;
> > > +
> > > + *retlen = sizeof(*buf);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int macronix_30lfxg18ac_otp_enable(struct nand_chip *nand)
> > > +{
> > > + u8 feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
> > > +
> > > + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN;
> > > + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
> > > + feature_buf);
> > > +}
> > > +
> > > +static int macronix_30lfxg18ac_otp_disable(struct nand_chip *nand)
> > > +{
> > > + u8 feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
> > > +
> > > + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
> > > + feature_buf);
> > > +}
> > > +
> > > +static int __macronix_30lfxg18ac_rw_otp(struct mtd_info *mtd,
> > > + loff_t offs_in_flash,
> > > + size_t len, size_t *retlen,
> > > + u_char *buf, bool write)
> > > +{
> > > + struct nand_chip *nand;
> > > + size_t bytes_handled;
> > > + off_t offs_in_page;
> > > + void *dma_buf;
> > > + u64 page;
> > > + int ret;
> > > +
> > > + /* 'nand_prog/read_page_op()' may use 'buf' as DMA buffer,
> > > + * so allocate properly aligned memory for it. This is
> > > + * needed because cross page accesses may lead to unaligned
> > > + * buffer address for DMA.
> > > + */
> > > + dma_buf = kmalloc(MACRONIX_30LFXG18AC_OTP_PAGE_SIZE, GFP_KERNEL);
> > > + if (!dma_buf)
> > > + return -ENOMEM;
> > > +
> > > + nand = mtd_to_nand(mtd);
> > > + nand_select_target(nand, 0);
> > > +
> > > + ret = macronix_30lfxg18ac_otp_enable(nand);
> > > + if (ret)
> > > + goto out_otp;
> > > +
> > > + page = offs_in_flash;
> > > + /* 'page' will be result of division. */
> > > + offs_in_page = do_div(page, MACRONIX_30LFXG18AC_OTP_PAGE_SIZE);
> > > + bytes_handled = 0;
> > > +
> > > + while (bytes_handled < len &&
> > > + page < MACRONIX_30LFXG18AC_OTP_PAGES) {
> > > + size_t bytes_to_handle;
> > > +
> > > + bytes_to_handle = min_t(size_t, len - bytes_handled,
> > > + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE -
> > > + offs_in_page);
> > > +
> > > + if (write) {
> > > + memcpy(dma_buf, &buf[bytes_handled], bytes_to_handle);
> > > + ret = nand_prog_page_op(nand, page, offs_in_page,
> > > + dma_buf, bytes_to_handle);
> > > + } else {
> > > + ret = nand_read_page_op(nand, page, offs_in_page,
> > > + dma_buf, bytes_to_handle);
> > > + if (!ret)
> > > + memcpy(&buf[bytes_handled], dma_buf,
> > > + bytes_to_handle);
> > > + }
> > > + if (ret)
> > > + goto out_otp;
> > > +
> > > + bytes_handled += bytes_to_handle;
> > > + offs_in_page = 0;
> > > + page++;
> > > + }
> > > +
> > > + *retlen = bytes_handled;
> > > +
> > > +out_otp:
> > > + if (ret)
> > > + dev_err(&mtd->dev, "failed to perform OTP IO: %i\n", ret);
> > > +
> > > + ret = macronix_30lfxg18ac_otp_disable(nand);
> > > + if (ret)
> > > + dev_err(&mtd->dev, "failed to leave OTP mode after %s\n",
> > > + write ? "write" : "read");
> > > +
> > > + nand_deselect_target(nand);
> > > + kfree(dma_buf);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int macronix_30lfxg18ac_write_otp(struct mtd_info *mtd, loff_t to,
> > > + size_t len, size_t *rlen,
> > > + const u_char *buf)
> > > +{
> > > + return __macronix_30lfxg18ac_rw_otp(mtd, to, len, rlen, (u_char *)buf,
> > > + true);
> > > +}
> > > +
> > > +static int macronix_30lfxg18ac_read_otp(struct mtd_info *mtd, loff_t from,
> > > + size_t len, size_t *rlen,
> > > + u_char *buf)
> > > +{
> > > + return __macronix_30lfxg18ac_rw_otp(mtd, from, len, rlen, buf, false);
> > > +}
> > > +
> > > +static int macronix_30lfxg18ac_lock_otp(struct mtd_info *mtd, loff_t from,
> > > + size_t len)
> > > +{
> > > + u8 feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
> > > + struct nand_chip *nand;
> > > + int ret;
> > > +
> > > + if (from != MACRONIX_30LFXG18AC_OTP_START_BYTE ||
> > > + len != MACRONIX_30LFXG18AC_OTP_SIZE_BYTES)
> > > + return -EINVAL;
> > > +
> > > + dev_dbg(&mtd->dev, "locking OTP\n");
> > > +
> > > + nand = mtd_to_nand(mtd);
> > > + nand_select_target(nand, 0);
> > > +
> > > + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN |
> > > + MACRONIX_30LFXG18AC_OTP_LOCKED;
> > > + ret = nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
> > > + feature_buf);
> > > + if (ret) {
> > > + dev_err(&mtd->dev,
> > > + "failed to lock OTP (set features): %i\n", ret);
> > > + nand_deselect_target(nand);
> > > + return ret;
> > > + }
> > > +
> > > + /* Do dummy page prog with zero address. */
> > > + feature_buf[0] = 0;
> > > + ret = nand_prog_page_op(nand, 0, 0, feature_buf, 1);
> > > + if (ret)
> > > + dev_err(&mtd->dev,
> > > + "failed to lock OTP (page prog): %i\n", ret);
> > > +
> > > + ret = macronix_30lfxg18ac_otp_disable(nand);
> > > + if (ret)
> > > + dev_err(&mtd->dev, "failed to leave OTP mode after lock\n");
> > > +
> > > + nand_deselect_target(nand);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static void macronix_nand_setup_otp(struct nand_chip *chip)
> > > +{
> > > + static const char * const supported_otp_models[] = {
> > > + "MX30LF1G18AC",
> > > + "MX30LF2G18AC",
> > > + "MX30LF4G18AC",
> > > + };
> > > + struct mtd_info *mtd;
> > > +
> > > + if (match_string(supported_otp_models,
> > > + ARRAY_SIZE(supported_otp_models),
> > > + chip->parameters.model) < 0)
> > > + return;
> > > +
> > > + if (!chip->parameters.supports_set_get_features)
> > > + return;
> > > +
> > > + bitmap_set(chip->parameters.get_feature_list,
> > > + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
> > > + bitmap_set(chip->parameters.set_feature_list,
> > > + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
> > > +
> > > + mtd = nand_to_mtd(chip);
> > > + mtd->_get_fact_prot_info = macronix_30lfxg18ac_get_otp_info;
> > > + mtd->_read_fact_prot_reg = macronix_30lfxg18ac_read_otp;
> > > + mtd->_get_user_prot_info = macronix_30lfxg18ac_get_otp_info;
> > > + mtd->_read_user_prot_reg = macronix_30lfxg18ac_read_otp;
> > > + mtd->_write_user_prot_reg = macronix_30lfxg18ac_write_otp;
> > > + mtd->_lock_user_prot_reg = macronix_30lfxg18ac_lock_otp;
> > > +}
> > > +
> > > static int macronix_nand_init(struct nand_chip *chip)
> > > {
> > > if (nand_is_slc(chip))
> > > @@ -325,6 +540,7 @@ static int macronix_nand_init(struct nand_chip *chip)
> > > macronix_nand_onfi_init(chip);
> > > macronix_nand_block_protection_support(chip);
> > > macronix_nand_deep_power_down_support(chip);
> > > + macronix_nand_setup_otp(chip);
> > >
> > > return 0;
> > > }
> >
> >
> > Thanks,
> > Miquèl
>
> Thanks
> Jaime
^ permalink raw reply
* Re: [PATCH v4] mtd: rawnand: macronix: OTP access for MX30LFxG18AC
From: liao jaime @ 2023-06-14 9:06 UTC (permalink / raw)
To: Miquel Raynal
Cc: Arseniy Krasnov, Richard Weinberger, Vignesh Raghavendra,
Sumit Semwal, Christian König, oxffffaa, kernel,
Boris Brezillon, Jaime Liao, Mason Yang, linux-mtd, linux-kernel,
linux-media, dri-devel, linaro-mm-sig
In-Reply-To: <20230612185354.09b88e0d@xps-13>
Hi Miquel
>
> Hello,
>
> AVKrasnov@sberdevices.ru wrote on Tue, 23 May 2023 13:16:34 +0300:
>
> > This adds support for OTP area access on MX30LFxG18AC chip series.
>
> Jaime, any feedback on this? Will you test it?
>
> How are we supposed to test the OTP is locked? I see this is still an
> open point.
After checking with internal, sub feature parameter are volatile register.
It could be change after enter/exit OTP region or power cycle even OTP
region have been locked.
OTP operation mode still could be enter/exit and region is read only
after OTP in protect mode.
#program command could execute but no use after setting OTP region in
protect mode.
So that we can't check whether OTP region is locked via get feature.
And we don't have region for checking status of OTP locked.
>
> >
> > Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> > ---
> > v1 -> v2:
> > * Add slab.h include due to kernel test robot error.
> > v2 -> v3:
> > * Use 'uint64_t' as input argument for 'do_div()' instead
> > of 'unsigned long' due to kernel test robot error.
> > v3 -> v4:
> > * Use 'dev_err()' instead of 'WARN()'.
> > * Call 'match_string()' before checking 'supports_set_get_features'
> > in 'macronix_nand_setup_otp().
> > * Use 'u8' instead of 'uint8_t' as ./checkpatch.pl wants.
> >
> > drivers/mtd/nand/raw/nand_macronix.c | 216 +++++++++++++++++++++++++++
> > 1 file changed, 216 insertions(+)
> >
> > diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c
> > index 1472f925f386..be1ffa93bebb 100644
> > --- a/drivers/mtd/nand/raw/nand_macronix.c
> > +++ b/drivers/mtd/nand/raw/nand_macronix.c
> > @@ -6,6 +6,7 @@
> > * Author: Boris Brezillon <boris.brezillon@free-electrons.com>
> > */
> >
> > +#include <linux/slab.h>
> > #include "linux/delay.h"
> > #include "internals.h"
> >
> > @@ -31,6 +32,20 @@
> >
> > #define MXIC_CMD_POWER_DOWN 0xB9
> >
> > +#define ONFI_FEATURE_ADDR_30LFXG18AC_OTP 0x90
> > +#define MACRONIX_30LFXG18AC_OTP_START_PAGE 0
> > +#define MACRONIX_30LFXG18AC_OTP_PAGES 30
> > +#define MACRONIX_30LFXG18AC_OTP_PAGE_SIZE 2112
> > +#define MACRONIX_30LFXG18AC_OTP_START_BYTE \
> > + (MACRONIX_30LFXG18AC_OTP_START_PAGE * \
> > + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
> > +#define MACRONIX_30LFXG18AC_OTP_SIZE_BYTES \
> > + (MACRONIX_30LFXG18AC_OTP_PAGES * \
> > + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
> > +
> > +#define MACRONIX_30LFXG18AC_OTP_EN BIT(0)
> > +#define MACRONIX_30LFXG18AC_OTP_LOCKED BIT(1)
> > +
> > struct nand_onfi_vendor_macronix {
> > u8 reserved;
> > u8 reliability_func;
> > @@ -316,6 +331,206 @@ static void macronix_nand_deep_power_down_support(struct nand_chip *chip)
> > chip->ops.resume = mxic_nand_resume;
> > }
> >
> > +static int macronix_30lfxg18ac_get_otp_info(struct mtd_info *mtd, size_t len,
> > + size_t *retlen,
> > + struct otp_info *buf)
> > +{
> > + if (len < sizeof(*buf))
> > + return -EINVAL;
> > +
> > + /* Don't know how to check that OTP is locked. */
> > + buf->locked = 0;
> > + buf->start = MACRONIX_30LFXG18AC_OTP_START_BYTE;
> > + buf->length = MACRONIX_30LFXG18AC_OTP_SIZE_BYTES;
> > +
> > + *retlen = sizeof(*buf);
> > +
> > + return 0;
> > +}
> > +
> > +static int macronix_30lfxg18ac_otp_enable(struct nand_chip *nand)
> > +{
> > + u8 feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
> > +
> > + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN;
> > + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
> > + feature_buf);
> > +}
> > +
> > +static int macronix_30lfxg18ac_otp_disable(struct nand_chip *nand)
> > +{
> > + u8 feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
> > +
> > + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
> > + feature_buf);
> > +}
> > +
> > +static int __macronix_30lfxg18ac_rw_otp(struct mtd_info *mtd,
> > + loff_t offs_in_flash,
> > + size_t len, size_t *retlen,
> > + u_char *buf, bool write)
> > +{
> > + struct nand_chip *nand;
> > + size_t bytes_handled;
> > + off_t offs_in_page;
> > + void *dma_buf;
> > + u64 page;
> > + int ret;
> > +
> > + /* 'nand_prog/read_page_op()' may use 'buf' as DMA buffer,
> > + * so allocate properly aligned memory for it. This is
> > + * needed because cross page accesses may lead to unaligned
> > + * buffer address for DMA.
> > + */
> > + dma_buf = kmalloc(MACRONIX_30LFXG18AC_OTP_PAGE_SIZE, GFP_KERNEL);
> > + if (!dma_buf)
> > + return -ENOMEM;
> > +
> > + nand = mtd_to_nand(mtd);
> > + nand_select_target(nand, 0);
> > +
> > + ret = macronix_30lfxg18ac_otp_enable(nand);
> > + if (ret)
> > + goto out_otp;
> > +
> > + page = offs_in_flash;
> > + /* 'page' will be result of division. */
> > + offs_in_page = do_div(page, MACRONIX_30LFXG18AC_OTP_PAGE_SIZE);
> > + bytes_handled = 0;
> > +
> > + while (bytes_handled < len &&
> > + page < MACRONIX_30LFXG18AC_OTP_PAGES) {
> > + size_t bytes_to_handle;
> > +
> > + bytes_to_handle = min_t(size_t, len - bytes_handled,
> > + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE -
> > + offs_in_page);
> > +
> > + if (write) {
> > + memcpy(dma_buf, &buf[bytes_handled], bytes_to_handle);
> > + ret = nand_prog_page_op(nand, page, offs_in_page,
> > + dma_buf, bytes_to_handle);
> > + } else {
> > + ret = nand_read_page_op(nand, page, offs_in_page,
> > + dma_buf, bytes_to_handle);
> > + if (!ret)
> > + memcpy(&buf[bytes_handled], dma_buf,
> > + bytes_to_handle);
> > + }
> > + if (ret)
> > + goto out_otp;
> > +
> > + bytes_handled += bytes_to_handle;
> > + offs_in_page = 0;
> > + page++;
> > + }
> > +
> > + *retlen = bytes_handled;
> > +
> > +out_otp:
> > + if (ret)
> > + dev_err(&mtd->dev, "failed to perform OTP IO: %i\n", ret);
> > +
> > + ret = macronix_30lfxg18ac_otp_disable(nand);
> > + if (ret)
> > + dev_err(&mtd->dev, "failed to leave OTP mode after %s\n",
> > + write ? "write" : "read");
> > +
> > + nand_deselect_target(nand);
> > + kfree(dma_buf);
> > +
> > + return ret;
> > +}
> > +
> > +static int macronix_30lfxg18ac_write_otp(struct mtd_info *mtd, loff_t to,
> > + size_t len, size_t *rlen,
> > + const u_char *buf)
> > +{
> > + return __macronix_30lfxg18ac_rw_otp(mtd, to, len, rlen, (u_char *)buf,
> > + true);
> > +}
> > +
> > +static int macronix_30lfxg18ac_read_otp(struct mtd_info *mtd, loff_t from,
> > + size_t len, size_t *rlen,
> > + u_char *buf)
> > +{
> > + return __macronix_30lfxg18ac_rw_otp(mtd, from, len, rlen, buf, false);
> > +}
> > +
> > +static int macronix_30lfxg18ac_lock_otp(struct mtd_info *mtd, loff_t from,
> > + size_t len)
> > +{
> > + u8 feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
> > + struct nand_chip *nand;
> > + int ret;
> > +
> > + if (from != MACRONIX_30LFXG18AC_OTP_START_BYTE ||
> > + len != MACRONIX_30LFXG18AC_OTP_SIZE_BYTES)
> > + return -EINVAL;
> > +
> > + dev_dbg(&mtd->dev, "locking OTP\n");
> > +
> > + nand = mtd_to_nand(mtd);
> > + nand_select_target(nand, 0);
> > +
> > + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN |
> > + MACRONIX_30LFXG18AC_OTP_LOCKED;
> > + ret = nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
> > + feature_buf);
> > + if (ret) {
> > + dev_err(&mtd->dev,
> > + "failed to lock OTP (set features): %i\n", ret);
> > + nand_deselect_target(nand);
> > + return ret;
> > + }
> > +
> > + /* Do dummy page prog with zero address. */
> > + feature_buf[0] = 0;
> > + ret = nand_prog_page_op(nand, 0, 0, feature_buf, 1);
> > + if (ret)
> > + dev_err(&mtd->dev,
> > + "failed to lock OTP (page prog): %i\n", ret);
> > +
> > + ret = macronix_30lfxg18ac_otp_disable(nand);
> > + if (ret)
> > + dev_err(&mtd->dev, "failed to leave OTP mode after lock\n");
> > +
> > + nand_deselect_target(nand);
> > +
> > + return ret;
> > +}
> > +
> > +static void macronix_nand_setup_otp(struct nand_chip *chip)
> > +{
> > + static const char * const supported_otp_models[] = {
> > + "MX30LF1G18AC",
> > + "MX30LF2G18AC",
> > + "MX30LF4G18AC",
> > + };
> > + struct mtd_info *mtd;
> > +
> > + if (match_string(supported_otp_models,
> > + ARRAY_SIZE(supported_otp_models),
> > + chip->parameters.model) < 0)
> > + return;
> > +
> > + if (!chip->parameters.supports_set_get_features)
> > + return;
> > +
> > + bitmap_set(chip->parameters.get_feature_list,
> > + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
> > + bitmap_set(chip->parameters.set_feature_list,
> > + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
> > +
> > + mtd = nand_to_mtd(chip);
> > + mtd->_get_fact_prot_info = macronix_30lfxg18ac_get_otp_info;
> > + mtd->_read_fact_prot_reg = macronix_30lfxg18ac_read_otp;
> > + mtd->_get_user_prot_info = macronix_30lfxg18ac_get_otp_info;
> > + mtd->_read_user_prot_reg = macronix_30lfxg18ac_read_otp;
> > + mtd->_write_user_prot_reg = macronix_30lfxg18ac_write_otp;
> > + mtd->_lock_user_prot_reg = macronix_30lfxg18ac_lock_otp;
> > +}
> > +
> > static int macronix_nand_init(struct nand_chip *chip)
> > {
> > if (nand_is_slc(chip))
> > @@ -325,6 +540,7 @@ static int macronix_nand_init(struct nand_chip *chip)
> > macronix_nand_onfi_init(chip);
> > macronix_nand_block_protection_support(chip);
> > macronix_nand_deep_power_down_support(chip);
> > + macronix_nand_setup_otp(chip);
> >
> > return 0;
> > }
>
>
> Thanks,
> Miquèl
Thanks
Jaime
^ permalink raw reply
* RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
From: Biju Das @ 2023-06-14 8:21 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Wolfram Sang, Geert Uytterhoeven, Alexandre Belloni, Mark Brown,
Krzysztof Kozlowski, Rob Herring, Conor Dooley, Andrzej Hajda,
Neil Armstrong, Robert Foss, David Airlie, Daniel Vetter,
Kieran Bingham, Mauro Carvalho Chehab, Hans Verkuil,
Alessandro Zummo, Jonas Karlman, Jernej Skrabec,
Uwe Kleine-König, Corey Minyard, Marek Behún,
Jiasheng Jiang, Antonio Borneo, Abhinav Kumar, Ahmad Fatoum,
dri-devel@lists.freedesktop.org, linux-i2c@vger.kernel.org,
linux-media@vger.kernel.org, Fabrizio Castro,
linux-renesas-soc@vger.kernel.org
In-Reply-To: <20230614081314.GD17519@pendragon.ideasonboard.com>
Hi Laurent,
Thanks for the feedback.
> Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
>
> On Tue, Jun 13, 2023 at 07:31:46PM +0000, Biju Das wrote:
> > > Subject: RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device
> > > API
> > > > Subject: RE: [PATCH v5 01/11] i2c: Enhance
> > > > i2c_new_ancillary_device API
> > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance
> > > > > i2c_new_ancillary_device API
> > > > >
> > > > > Hi everyone,
> > > > >
> > > > > > Perhaps we should first think through what an ancillary device
> > > > > > really is. My understanding is that it is used to talk to
> > > > > > secondary addresses of a multi-address I2C slave device.
> > > > >
> > > > > As I mentioned somewhere before, this is not the case. Ancillary
> > > > > devices are when one *driver* handles more than one address.
> > > > > Everything else has been handled differently in the past (for
> > > > > all the uses I am aware of).
> > > > >
> > > > > Yet, I have another idea which is so simple that I wonder if it
> > > > > maybe has already been discussed so far?
> > > > >
> > > > > * have two regs in the bindings
> > > >
> > > > OK, it is inline with DT maintainers expectation as it is matching
> > > > with real hw as single device node having two regs.
> > > >
> > > > > * use the second reg with i2c_new_client_device to instantiate the
> > > > > RTC sibling. 'struct i2c_board_info', which is one parameter,
> should
> > > > > have enough options to pass data, e.g it has a software_node.
> > > >
> > > > OK, I can see the below can be passed from PMIC to new client
> device.
> > > >
> > > > client->addr = info->addr;
> > > >
> > > > client->init_irq = info->irq;
> > > >
> > > > >
> > > > > Should work or did I miss something here?
> > > >
> > > > I guess it will work. We instantiate appropriate device based On
> > > > PMIC revision and slave address and IRQ resource passed through
> > > > 'struct i2c_board_info'
> > > >
> > > > Will check this and update you.
> > >
> > > info.irq = irq; -->Irq fine
> > > info.addr = addr; -->slave address fine size = strscpy(info.type,
> > > name, sizeof(info.type)); -->instantiation based on PMIC version
> > > fine.
> > >
> > > 1) How do we share clk details on instantiated device to find is it
> > > connected to external crystal or external clock source? as we cannot
> > > pass of_node between PMIC and "i2c_board_info" as it results in
> > > pinctrl failure. info->platformdata and
> > > Client->dev.platformdata to retrieve this info??
> >
> > Or
> >
> > I2C instantiation based on actual oscillator bit value, ie, two
> > i2c_device_id's with one for setting oscillator bit and another for
> > clearing oscillator bit
> >
> > PMIC driver parses the clock details. Based on firmware version and
> > clock, It instantiates either i2c_device_id with setting oscillator
> > bit or clearing oscillator bit.
>
> I don't like that hack. I still think that two DT nodes is the best
> option, I think you're trying hard to hack around a problem that is
> actually not a problem.
Why do you think it is a hack? I believe rather it is actual solution
PMIC is a single device, with 2 regs, clocks, pinctrl and IRQ properties.
So it will be represented as single node with single compatible.
By instating a client device, we are sharing the relevant resources to RTC device driver.
Cheers,
Biju
^ permalink raw reply
* Fotowoltaika- propozycja instalacji
From: Norbert Karecki @ 2023-06-14 8:10 UTC (permalink / raw)
To: linux-media
Dzień dobry,
Czy rozważali Państwo montaż systemu fotowoltaicznego?
Instalacja fotowoltaiczna jest najlepszym sposobem na obniżenie wysokości rachunków za prąd (pozostają tylko opłaty stałe) i zabezpieczenie się przed rosnącymi cenami energii elektrycznej. Jest to w pełni odnawialne i bezemisyjne źródło energii, dzięki czemu przyczyniamy się do ochrony środowiska naturalnego.
Działamy od wielu lat na rynku energetycznym. Przygotujemy projekt, wycenę oraz kompleksowo wykonamy i zgłosimy realizację do zakładu energetycznego.
Czy chcą Państwo poznać naszą propozycję?
Pozdrawiam,
Norbert Karecki
^ permalink raw reply
* Re: [PATCH v2 3/5] media: mediatek: vcodec: Read HW active status from clock
From: AngeloGioacchino Del Regno @ 2023-06-14 8:13 UTC (permalink / raw)
To: Stephen Boyd, Chen-Yu Tsai, Nícolas F. R. A. Prado
Cc: Matthias Brugger, Hans Verkuil, kernel, Andrew-CT Chen,
Mauro Carvalho Chehab, Tiffany Lin, Yunfei Dong, linux-arm-kernel,
linux-kernel, linux-media, linux-mediatek
In-Reply-To: <d579dc00ed9877f9daf170134fe781e6.sboyd@kernel.org>
Il 12/06/23 21:19, Stephen Boyd ha scritto:
> Quoting AngeloGioacchino Del Regno (2023-06-09 00:42:13)
>> Il 09/06/23 01:56, Stephen Boyd ha scritto:
>>> Quoting AngeloGioacchino Del Regno (2023-06-08 02:01:58)
>>>> Il 08/06/23 10:12, Chen-Yu Tsai ha scritto:
>>>>> On Thu, Jun 8, 2023 at 4:57 AM Nícolas F. R. A. Prado
>>>>> <nfraprado@collabora.com> wrote:
>>>>>> diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
>>>>>> index 9c652beb3f19..8038472fb67b 100644
>>>>>> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
>>>>>> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
>>>>>
>>>>> AFAIK this is still around for clk drivers that haven't moved to clk_hw.
>>>>> It shouldn't be used by clock consumers. Would it be better to just pass
>>>>> a syscon?
>>>>>
>>>>
>>>> This is a legit usage of __clk_is_enabled().... because that's what we're really
>>>> doing here, we're checking if a clock got enabled by the underlying MCU (as that
>>>> clock goes up after the VDEC boots).
>>>>
>>>> If this is *not* acceptable as it is, we will have to add a clock API call to
>>>> check if a clock is enabled... but it didn't seem worth doing since we don't
>>>> expect anyone else to have any legit usage of that, or at least, we don't know
>>>> about anyone else needing that...
>>>
>>> The design of the clk.h API has been that no clk consumer should need to
>>> find out if a clk is enabled. Instead, the clk consumer should enable
>>> the clk if they want it enabled. Is there no other way to know that the
>>> vcodec hardware is active?
>>>
>>
>> The firmware gives an indication of "boot done", but that's for the "core" part
>> of the vcodec... then it manages this clock internally to enable/disable the
>> "compute" IP of the decoder.
>>
>> As far as I know (and I've been researching about this) the firmware will not
>> give any "decoder powered, clocked - ready to get data" indication, and the
>> only way that we have to judge whether it is in this specific state or not is
>> to check if the "VDEC_ACTIVE" clock got enabled by the firmware.
>
> Is Linux ever going to use clk consumer APIs like clk_enable/clk_disable
> on this VDEC_ACTIVE clk? If the answer is no, then there isn't any
> reason to put it in the clk framework, and probably syscon is the way to
> go for now.
>
Not for the current platform, but that may change in future SoCs... we're not sure.
> Another approach could be to wait for some amount of time after telling
> firmware to power up and assume the hardware is active.
>
That would be highly error prone though. Expecting that the HW is alive means that
we're 100% sure that both firmware and driver are doing the right thing at every
moment, which is something that we'd like to assume but, realistically, for safety
reasons we just don't.
Should we anyway go for a syscon *now* and then change it to a clock later, if any
new platform needs this as a clock?
I'm in doubt now on how to proceed.
> ----
>
> I see that the __clk_is_enabled() API is being used in some other
> consumer drivers. I think at one point we were down to one or two users.
> I'll try to remove this function entirely, but it will still be possible
> to get at the clk_hw for a clk with __clk_get_hw() and then call
> clk_hw_is_enabled().
>
Makes sense.
Regards,
Angelo
^ permalink raw reply
* Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
From: Laurent Pinchart @ 2023-06-14 8:13 UTC (permalink / raw)
To: Biju Das
Cc: Wolfram Sang, Geert Uytterhoeven, Alexandre Belloni, Mark Brown,
Krzysztof Kozlowski, Rob Herring, Conor Dooley, Andrzej Hajda,
Neil Armstrong, Robert Foss, David Airlie, Daniel Vetter,
Kieran Bingham, Mauro Carvalho Chehab, Hans Verkuil,
Alessandro Zummo, Jonas Karlman, Jernej Skrabec,
Uwe Kleine-König, Corey Minyard, Marek Behún,
Jiasheng Jiang, Antonio Borneo, Abhinav Kumar, Ahmad Fatoum,
dri-devel@lists.freedesktop.org, linux-i2c@vger.kernel.org,
linux-media@vger.kernel.org, Fabrizio Castro,
linux-renesas-soc@vger.kernel.org
In-Reply-To: <OS0PR01MB59220D794AED55A6B795C3EF8655A@OS0PR01MB5922.jpnprd01.prod.outlook.com>
On Tue, Jun 13, 2023 at 07:31:46PM +0000, Biju Das wrote:
> > Subject: RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
> > > Subject: RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
> > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
> > > >
> > > > Hi everyone,
> > > >
> > > > > Perhaps we should first think through what an ancillary device
> > > > > really is. My understanding is that it is used to talk to
> > > > > secondary addresses of a multi-address I2C slave device.
> > > >
> > > > As I mentioned somewhere before, this is not the case. Ancillary
> > > > devices are when one *driver* handles more than one address.
> > > > Everything else has been handled differently in the past (for all
> > > > the uses I am aware of).
> > > >
> > > > Yet, I have another idea which is so simple that I wonder if it
> > > > maybe has already been discussed so far?
> > > >
> > > > * have two regs in the bindings
> > >
> > > OK, it is inline with DT maintainers expectation as it is matching
> > > with real hw as single device node having two regs.
> > >
> > > > * use the second reg with i2c_new_client_device to instantiate the
> > > > RTC sibling. 'struct i2c_board_info', which is one parameter, should
> > > > have enough options to pass data, e.g it has a software_node.
> > >
> > > OK, I can see the below can be passed from PMIC to new client device.
> > >
> > > client->addr = info->addr;
> > >
> > > client->init_irq = info->irq;
> > >
> > > >
> > > > Should work or did I miss something here?
> > >
> > > I guess it will work. We instantiate appropriate device based On PMIC
> > > revision and slave address and IRQ resource passed through 'struct
> > > i2c_board_info'
> > >
> > > Will check this and update you.
> >
> > info.irq = irq; -->Irq fine
> > info.addr = addr; -->slave address fine
> > size = strscpy(info.type, name, sizeof(info.type)); -->instantiation based
> > on PMIC version fine.
> >
> > 1) How do we share clk details on instantiated device to find is it
> > connected to external crystal or external clock source? as we cannot pass
> > of_node between PMIC and "i2c_board_info" as it results in pinctrl
> > failure. info->platformdata and
> > Client->dev.platformdata to retrieve this info??
>
> Or
>
> I2C instantiation based on actual oscillator bit value, ie, two i2c_device_id's
> with one for setting oscillator bit and another for clearing oscillator bit
>
> PMIC driver parses the clock details. Based on firmware version and clock,
> It instantiates either i2c_device_id with setting oscillator bit or
> clearing oscillator bit.
I don't like that hack. I still think that two DT nodes is the best
option, I think you're trying hard to hack around a problem that is
actually not a problem.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
From: Biju Das @ 2023-06-14 8:02 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Wolfram Sang, Krzysztof Kozlowski, Laurent Pinchart, Rob Herring,
Conor Dooley, Andrzej Hajda, Neil Armstrong, Robert Foss,
David Airlie, Daniel Vetter, Kieran Bingham,
Mauro Carvalho Chehab, Hans Verkuil, Alessandro Zummo,
Alexandre Belloni, Jonas Karlman, Jernej Skrabec,
Uwe Kleine-König, Corey Minyard, Marek Behún,
Jiasheng Jiang, Antonio Borneo, Abhinav Kumar, Ahmad Fatoum,
dri-devel@lists.freedesktop.org, linux-i2c@vger.kernel.org,
linux-media@vger.kernel.org, Fabrizio Castro,
linux-renesas-soc@vger.kernel.org, Mark Brown
In-Reply-To: <CAMuHMdUhaSKiuVkmoYt1sm87emFZu7HSSCK-e95-Yy=g8Sgo4w@mail.gmail.com>
Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
>
> Hi Biju,
>
> On Tue, Jun 13, 2023 at 6:11 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device
> > > API On Tue, Jun 13, 2023 at 12:45 PM Biju Das
> > > <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance
> > > > > i2c_new_ancillary_device API On Mon, Jun 12, 2023 at 10:43 PM
> > > > > Wolfram Sang <wsa@kernel.org>
> > > wrote:
> > > > > > > Perhaps we should first think through what an ancillary
> > > > > > > device really is. My understanding is that it is used to
> > > > > > > talk to secondary addresses of a multi-address I2C slave
> device.
> > > > > >
> > > > > > As I mentioned somewhere before, this is not the case.
> > > > > > Ancillary devices are when one *driver* handles more than one
> address.
> > > > > > Everything else has been handled differently in the past (for
> > > > > > all the
> > > > > uses I am aware of).
> > > > > >
> > > > > > Yet, I have another idea which is so simple that I wonder if
> > > > > > it maybe has already been discussed so far?
> > > > > >
> > > > > > * have two regs in the bindings
> > > > > > * use the second reg with i2c_new_client_device to instantiate
> the
> > > > > > RTC sibling. 'struct i2c_board_info', which is one
> > > > > > parameter,
> > > should
> > > > > > have enough options to pass data, e.g it has a software_node.
> > > > > >
> > > > > > Should work or did I miss something here?
> > > > >
> > > > > That should work, mostly (i2c_new_dummy_device() also calls
> > > > > i2c_new_client_device()). And as i2c_board_info has an of_node
> > > > > member (something I had missed before!), the new I2C device can
> > > > > access the clocks in the DT node using the standard way.
> > > >
> > > > Looks like, I cannot assign of_node member like below as it
> > > > results in pinctrl failure[1] during device bind.
> > > >
> > > > info.of_node = client->dev.of_node;
> > > >
> > > > [1]
> > > > pinctrl-rzg2l 11030000.pinctrl: pin P43_0 already requested by
> > > > 3-0012; cannot claim for 3-006f pinctrl-rzg2l 11030000.pinctrl:
> > > > pin-344
> > > > (3-006f) status -22 pinctrl-rzg2l 11030000.pinctrl: could not
> > > > request pin 344 (P43_0) from group pmic on device pinctrl-rzg2l
> > > > raa215300 3-006f: Error applying setting, reverse things back
> > >
> > > Where do you have a reference to pin P43_0 in your DT?
> >
> > The reference to pin P43_0 is added in the PMIC node.
> >
> > I have done modification on my board to test PMIC INT# on RZ/G2L SMARC
> > EVK by wiring R83 on SoM module and PMOD0 PIN7.
> >
> > > The last versions you posted did not have any pinctrl properties?
> >
> > By default, PMIC_INT# is not populated RZ/G2L SMARC EVK, so I haven't
> > added Support for PMIC_INT# for the patches posted till date.
> >
> > Yesterday I checked with HW people, is there a way to enable PMIC_INT#
> > and they told me to do the above HW modification.
> >
> > Today I found this issue, with this modified HW and PMIC INT# enabled
> > on the DT, while assigning of_node of PMIC with info.of_node. It is just
> a coincidence.
>
> IC.
>
> So you now have two Linux devices pointing to the same DT node, causing
> pinctrl issues...
>
> I know this won't solve the core issue, but what is the exact pintrl
> configuration you are using? Is this using a GPIO with interrupt
> capabilities, or a dedicated interrupt pin? In case of the former, you
> don't need a pinctrl property in DT, as the GPIO controller itself should
> take care of that by asking the pin controller to configure the pin
> properly through pinctrl_gpio_request().
I was testing with both. This issue is triggered while configuring IRQ4 as PMIC_INT#.
pmic_pins: pmic {
pinmux = <RZG2L_PORT_PINMUX(43, 0, 4)>; /* IRQ4 */
};
&i2c3 {
raa215300: pmic@12 {
pinctrl-0 = <&pmic_pins>;
pinctrl-names = "default";
compatible = "renesas,raa215300";
reg = <0x12>, <0x6f>;
reg-names = "main", "rtc";
clocks = <&x2>;
clock-names = "xin";
//interrupt-parent = <&pinctrl>;
//interrupts = <RZG2L_GPIO(43, 0) IRQ_TYPE_EDGE_FALLING>;
interrupt-parent = <&irqc>;
interrupts = <RZG2L_IRQ4 IRQ_TYPE_EDGE_FALLING>;
};
};
Cheers,
Biju
^ permalink raw reply
* Re: [PATCH] media: v4l2-core: Fix a potential resource leak in v4l2_fwnode_parse_link()
From: Sakari Ailus @ 2023-06-14 8:00 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Mauro Carvalho Chehab, linux-kernel, kernel-janitors, linux-media
In-Reply-To: <34b714b6-cb49-1a34-58f5-8b5ef0da2714@wanadoo.fr>
Hi Christophe,
On Tue, Jun 13, 2023 at 07:15:40PM +0200, Christophe JAILLET wrote:
> Le 13/06/2023 à 12:55, Sakari Ailus a écrit :
> > Hi Christophe,
> >
> > On Mon, May 29, 2023 at 08:17:18AM +0200, Christophe JAILLET wrote:
> > > 'fwnode is known to be NULL, at this point, so fwnode_handle_put() is a
> > > no-op.
> > >
> > > Release the reference taken from a previous fwnode_graph_get_port_parent()
> > > call instead.
> > >
> > > Fixes: ca50c197bd96 ("[media] v4l: fwnode: Support generic fwnode for parsing standardised properties")
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > ---
> > > /!\ THIS PATCH IS SPECULATIVE /!\
> > > review with care
> > > ---
> > > drivers/media/v4l2-core/v4l2-fwnode.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > index 049c2f2001ea..b7dd467c53fd 100644
> > > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > @@ -571,7 +571,7 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
> > > fwnode = fwnode_graph_get_remote_endpoint(fwnode);
> > > if (!fwnode) {
> > > - fwnode_handle_put(fwnode);
> > > + fwnode_handle_put(link->local_node);
> >
> > link->local_node also needs to be non-NULL for the successful case. The
> > condition should take that into account. Could you send v2 with that?
> >
> > > return -ENOLINK;
> > > }
> >
>
> Hi,
> something like below?
Ah, remote_node must be non-NULL, too, indeed. It was surprisingly broken.
>
> @@ -568,19 +568,25 @@ int v4l2_fwnode_parse_link(struct fwnode_handle
> *fwnode,
> link->local_id = fwep.id;
> link->local_port = fwep.port;
> link->local_node = fwnode_graph_get_port_parent(fwnode);
> + if (!link->local_node)
> + return -ENOLINK;
>
> fwnode = fwnode_graph_get_remote_endpoint(fwnode);
> - if (!fwnode) {
> - fwnode_handle_put(fwnode);
> - return -ENOLINK;
> - }
> + if (!fwnode)
> + goto err_put_local_node;
On error, fwnode needs to be put from this onwards, too.
But you can use a single label: fwnode_handle_put() is NULL-safe.
>
> fwnode_graph_parse_endpoint(fwnode, &fwep);
> link->remote_id = fwep.id;
> link->remote_port = fwep.port;
> link->remote_node = fwnode_graph_get_port_parent(fwnode);
> + if (!link->remote_node)
> + goto err_put_local_node;
>
> return 0;
> +
> +err_put_local_node:
> + fwnode_handle_put(link->local_node);
> + return -ENOLINK;
> }
> EXPORT_SYMBOL_GPL(v4l2_fwnode_parse_link);
--
Kind regards,
Sakari Ailus
^ permalink raw reply
* Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
From: Geert Uytterhoeven @ 2023-06-14 7:53 UTC (permalink / raw)
To: Biju Das
Cc: Wolfram Sang, Krzysztof Kozlowski, Laurent Pinchart, Rob Herring,
Conor Dooley, Andrzej Hajda, Neil Armstrong, Robert Foss,
David Airlie, Daniel Vetter, Kieran Bingham,
Mauro Carvalho Chehab, Hans Verkuil, Alessandro Zummo,
Alexandre Belloni, Jonas Karlman, Jernej Skrabec,
Uwe Kleine-König, Corey Minyard, Marek Behún,
Jiasheng Jiang, Antonio Borneo, Abhinav Kumar, Ahmad Fatoum,
dri-devel@lists.freedesktop.org, linux-i2c@vger.kernel.org,
linux-media@vger.kernel.org, Fabrizio Castro,
linux-renesas-soc@vger.kernel.org, Mark Brown
In-Reply-To: <OS0PR01MB59224D7C95B9B0037046FCF78655A@OS0PR01MB5922.jpnprd01.prod.outlook.com>
Hi Biju,
On Tue, Jun 13, 2023 at 6:11 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
> > On Tue, Jun 13, 2023 at 12:45 PM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device
> > > > API On Mon, Jun 12, 2023 at 10:43 PM Wolfram Sang <wsa@kernel.org>
> > wrote:
> > > > > > Perhaps we should first think through what an ancillary device
> > > > > > really is. My understanding is that it is used to talk to
> > > > > > secondary addresses of a multi-address I2C slave device.
> > > > >
> > > > > As I mentioned somewhere before, this is not the case. Ancillary
> > > > > devices are when one *driver* handles more than one address.
> > > > > Everything else has been handled differently in the past (for all
> > > > > the
> > > > uses I am aware of).
> > > > >
> > > > > Yet, I have another idea which is so simple that I wonder if it
> > > > > maybe has already been discussed so far?
> > > > >
> > > > > * have two regs in the bindings
> > > > > * use the second reg with i2c_new_client_device to instantiate the
> > > > > RTC sibling. 'struct i2c_board_info', which is one parameter,
> > should
> > > > > have enough options to pass data, e.g it has a software_node.
> > > > >
> > > > > Should work or did I miss something here?
> > > >
> > > > That should work, mostly (i2c_new_dummy_device() also calls
> > > > i2c_new_client_device()). And as i2c_board_info has an of_node
> > > > member (something I had missed before!), the new I2C device can
> > > > access the clocks in the DT node using the standard way.
> > >
> > > Looks like, I cannot assign of_node member like below as it results in
> > > pinctrl failure[1] during device bind.
> > >
> > > info.of_node = client->dev.of_node;
> > >
> > > [1]
> > > pinctrl-rzg2l 11030000.pinctrl: pin P43_0 already requested by 3-0012;
> > > cannot claim for 3-006f pinctrl-rzg2l 11030000.pinctrl: pin-344
> > > (3-006f) status -22 pinctrl-rzg2l 11030000.pinctrl: could not request
> > > pin 344 (P43_0) from group pmic on device pinctrl-rzg2l
> > > raa215300 3-006f: Error applying setting, reverse things back
> >
> > Where do you have a reference to pin P43_0 in your DT?
>
> The reference to pin P43_0 is added in the PMIC node.
>
> I have done modification on my board to test PMIC INT# on RZ/G2L SMARC EVK
> by wiring R83 on SoM module and PMOD0 PIN7.
>
> > The last versions you posted did not have any pinctrl properties?
>
> By default, PMIC_INT# is not populated RZ/G2L SMARC EVK, so I haven't added
> Support for PMIC_INT# for the patches posted till date.
>
> Yesterday I checked with HW people, is there a way to enable PMIC_INT#
> and they told me to do the above HW modification.
>
> Today I found this issue, with this modified HW and PMIC INT# enabled on the DT,
> while assigning of_node of PMIC with info.of_node. It is just a coincidence.
IC.
So you now have two Linux devices pointing to the same DT node,
causing pinctrl issues...
I know this won't solve the core issue, but what is the exact pintrl
configuration you are using? Is this using a GPIO with interrupt
capabilities, or a dedicated interrupt pin? In case of the former,
you don't need a pinctrl property in DT, as the GPIO controller itself
should take care of that by asking the pin controller to configure
the pin properly through pinctrl_gpio_request().
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [syzbot] kernel BUG in vmf_insert_pfn_prot
From: Dmitry Vyukov @ 2023-06-14 7:33 UTC (permalink / raw)
To: syzbot
Cc: airlied, airlied, christian.koenig, daniel.vetter, daniel.vetter,
daniel, dri-devel, javierm, linaro-mm-sig-owner, linaro-mm-sig,
linux-kernel, linux-media, maarten.lankhorst, mripard,
suijingfeng, sumit.semwal, syzkaller-bugs, tzimmermann, zackr
In-Reply-To: <0000000000006d819905fe07c52f@google.com>
On Tue, 13 Jun 2023 at 21:23, syzbot
<syzbot+2d4f8693f438d2bd4bdb@syzkaller.appspotmail.com> wrote:
>
> syzbot suspects this issue was fixed by commit:
>
> commit a5b44c4adb1699661d22e5152fb26885f30a2e4c
> Author: Thomas Zimmermann <tzimmermann@suse.de>
> Date: Mon Mar 20 15:07:44 2023 +0000
>
> drm/fbdev-generic: Always use shadow buffering
>
> bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=1025ee07280000
> start commit: 0326074ff465 Merge tag 'net-next-6.1' of git://git.kernel...
> git tree: upstream
> kernel config: https://syzkaller.appspot.com/x/.config?x=d323d85b1f8a4ed7
> dashboard link: https://syzkaller.appspot.com/bug?extid=2d4f8693f438d2bd4bdb
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14fd1182880000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17567514880000
>
> If the result looks correct, please mark the issue as fixed by replying with:
>
> #syz fix: drm/fbdev-generic: Always use shadow buffering
>
> For information about bisection process see: https://goo.gl/tpsmEJ#bisection
Looks reasonable:
#syz fix: drm/fbdev-generic: Always use shadow buffering
^ permalink raw reply
* [PATCH v1] add IVSC support for IPU bridge driver
From: Wentong Wu @ 2023-06-14 1:04 UTC (permalink / raw)
To: sakari.ailus, bingbu.cao, andriy.shevchenko, linux-media
Cc: zhifeng.wang, xiang.ye, tian.shu.qiu, Wentong Wu
Previously on ACPI platforms, sensors that are intended to be connected
to a IPU device for use with the ipu3-cio2 driver lacking the necessary
connection information in firmware. IPU bridge driver is to connect
sensors to IPU device via software nodes.
Currently IVSC located between IPU device and sensors is available in
existing commercial platforms from multiple OEMs. But the connection
information between them in firmware is also not enough to build V4L2
connection graph. This patch parses the connection properties from the
SSDB buffer in DSDT and build the connection using software nodes.
IVSC driver is based on MEI framework (previously known as HECI), it
has two MEI clients, MEI CSI and MEI ACE. Both clients are used to
communicate messages with IVSC firmware. Linux abstracts MEI client
as a device, whose bus type is MEI. And the device is addressed by a
GUID/UUID which is part of the device name of MEI client. After figured
out MEI CSI via the UUID composed device name, this patch setup the
connection between MEI CSI and IPU, and the connection between MEI CSI
and sensor via software nodes.
Signed-off-by: Wentong Wu <wentong.wu@intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/pci/intel/ipu-bridge.c | 191 ++++++++++++++++++++++++++++++++++-
drivers/media/pci/intel/ipu-bridge.h | 19 +++-
2 files changed, 205 insertions(+), 5 deletions(-)
diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 62daa8c..8b0755d 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -4,6 +4,7 @@
#include <linux/acpi.h>
#include <linux/device.h>
#include <linux/i2c.h>
+#include <linux/mei_cl_bus.h>
#include <linux/pci.h>
#include <linux/property.h>
#include <media/v4l2-fwnode.h>
@@ -11,6 +12,16 @@
#include "ipu-bridge.h"
/*
+ * 92335fcf-3203-4472-af93-7b4453ac29da
+ *
+ * Used to build MEI CSI device name to lookup MEI CSI device by
+ * device_find_child_by_name().
+ */
+#define MEI_CSI_UUID \
+ UUID_LE(0x92335FCF, 0x3203, 0x4472, \
+ 0xAF, 0x93, 0x7b, 0x44, 0x53, 0xAC, 0x29, 0xDA)
+
+/*
* Extend this array with ACPI Hardware IDs of devices known to be working
* plus the number of link-frequencies expected by their drivers, along with
* the frequency values in hertz. This is somewhat opportunistic way of adding
@@ -61,6 +72,60 @@ static const char * const ipu_vcm_types[] = {
"lc898212axb",
};
+/*
+ * Used to figure out IVSC device by ipu_bridge_check_ivsc_dev()
+ * instead of device and driver match to probe IVSC device.
+ */
+static const struct acpi_device_id ivsc_acpi_ids[] = {
+ { "INTC1059" },
+ { "INTC1095" },
+ { "INTC100A" },
+ { "INTC10CF" },
+};
+
+static int ipu_bridge_check_ivsc_dev(struct ipu_sensor *sensor,
+ struct acpi_device *sensor_adev)
+{
+ acpi_handle handle = acpi_device_handle(sensor_adev);
+ struct acpi_device *consumer, *adev;
+ uuid_le uuid = MEI_CSI_UUID;
+ struct device *csi_dev;
+ unsigned int i;
+ char name[64];
+
+ for (i = 0; i < ARRAY_SIZE(ivsc_acpi_ids); i++) {
+ const struct acpi_device_id *acpi_id = &ivsc_acpi_ids[i];
+
+ for_each_acpi_dev_match(adev, acpi_id->id, NULL, -1) {
+ /* camera sensor depends on IVSC in DSDT */
+ for_each_acpi_consumer_dev(adev, consumer)
+ if (consumer->handle == handle)
+ break;
+
+ if (!consumer)
+ continue;
+
+ snprintf(name, sizeof(name), "%s-%pUl",
+ dev_name(&adev->dev), &uuid);
+
+ csi_dev = device_find_child_by_name(&adev->dev, name);
+ if (!csi_dev) {
+ acpi_dev_put(adev);
+ dev_err(&adev->dev,
+ "Failed to find MEI CSI dev\n");
+ return -ENODEV;
+ }
+
+ sensor->csi_dev = csi_dev;
+ sensor->ivsc_adev = adev;
+
+ return 0;
+ }
+ }
+
+ return 0;
+}
+
static int ipu_bridge_read_acpi_buffer(struct acpi_device *adev, char *id,
void *data, u32 size)
{
@@ -137,16 +202,53 @@ static void ipu_bridge_create_fwnode_properties(
struct ipu_bridge *bridge,
const struct ipu_sensor_config *cfg)
{
- u32 rotation;
+ struct ipu_property_names *names = &sensor->prop_names;
+ struct software_node *nodes = sensor->swnodes;
enum v4l2_fwnode_orientation orientation;
+ u32 rotation;
rotation = ipu_bridge_parse_rotation(sensor);
orientation = ipu_bridge_parse_orientation(sensor);
sensor->prop_names = prop_names;
- sensor->local_ref[0] = SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_IPU_ENDPOINT]);
- sensor->remote_ref[0] = SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_SENSOR_ENDPOINT]);
+ if (sensor->csi_dev) {
+ sensor->local_ref[0] =
+ SOFTWARE_NODE_REFERENCE(&nodes[SWNODE_IVSC_SENSOR_ENDPOINT]);
+ sensor->remote_ref[0] =
+ SOFTWARE_NODE_REFERENCE(&nodes[SWNODE_IVSC_IPU_ENDPOINT]);
+ sensor->ivsc_sensor_ref[0] =
+ SOFTWARE_NODE_REFERENCE(&nodes[SWNODE_SENSOR_ENDPOINT]);
+ sensor->ivsc_ipu_ref[0] =
+ SOFTWARE_NODE_REFERENCE(&nodes[SWNODE_IPU_ENDPOINT]);
+
+ sensor->ivsc_sensor_ep_properties[0] =
+ PROPERTY_ENTRY_U32(names->bus_type,
+ V4L2_FWNODE_BUS_TYPE_CSI2_DPHY);
+ sensor->ivsc_sensor_ep_properties[1] =
+ PROPERTY_ENTRY_U32_ARRAY_LEN(names->data_lanes,
+ bridge->data_lanes,
+ sensor->ssdb.lanes);
+ sensor->ivsc_sensor_ep_properties[2] =
+ PROPERTY_ENTRY_REF_ARRAY(names->remote_endpoint,
+ sensor->ivsc_sensor_ref);
+
+ sensor->ivsc_ipu_ep_properties[0] =
+ PROPERTY_ENTRY_U32(names->bus_type,
+ V4L2_FWNODE_BUS_TYPE_CSI2_DPHY);
+ sensor->ivsc_ipu_ep_properties[1] =
+ PROPERTY_ENTRY_U32_ARRAY_LEN(names->data_lanes,
+ bridge->data_lanes,
+ sensor->ssdb.lanes);
+ sensor->ivsc_ipu_ep_properties[2] =
+ PROPERTY_ENTRY_REF_ARRAY(names->remote_endpoint,
+ sensor->ivsc_ipu_ref);
+ } else {
+ sensor->local_ref[0] =
+ SOFTWARE_NODE_REFERENCE(&nodes[SWNODE_IPU_ENDPOINT]);
+ sensor->remote_ref[0] =
+ SOFTWARE_NODE_REFERENCE(&nodes[SWNODE_SENSOR_ENDPOINT]);
+ }
sensor->dev_properties[0] = PROPERTY_ENTRY_U32(
sensor->prop_names.clock_frequency,
@@ -201,6 +303,15 @@ static void ipu_bridge_init_swnode_names(struct ipu_sensor *sensor)
snprintf(sensor->node_names.endpoint,
sizeof(sensor->node_names.endpoint),
SWNODE_GRAPH_ENDPOINT_NAME_FMT, 0); /* And endpoint 0 */
+
+ if (sensor->csi_dev) {
+ snprintf(sensor->node_names.ivsc_sensor_port,
+ sizeof(sensor->node_names.ivsc_sensor_port),
+ SWNODE_GRAPH_PORT_NAME_FMT, 0);
+ snprintf(sensor->node_names.ivsc_ipu_port,
+ sizeof(sensor->node_names.ivsc_ipu_port),
+ SWNODE_GRAPH_PORT_NAME_FMT, 1);
+ }
}
static void ipu_bridge_init_swnode_group(struct ipu_sensor *sensor)
@@ -214,11 +325,31 @@ static void ipu_bridge_init_swnode_group(struct ipu_sensor *sensor)
sensor->group[SWNODE_IPU_ENDPOINT] = &nodes[SWNODE_IPU_ENDPOINT];
if (sensor->ssdb.vcmtype)
sensor->group[SWNODE_VCM] = &nodes[SWNODE_VCM];
+
+ if (sensor->csi_dev) {
+ sensor->group[SWNODE_IVSC_HID] =
+ &nodes[SWNODE_IVSC_HID];
+ sensor->group[SWNODE_IVSC_SENSOR_PORT] =
+ &nodes[SWNODE_IVSC_SENSOR_PORT];
+ sensor->group[SWNODE_IVSC_SENSOR_ENDPOINT] =
+ &nodes[SWNODE_IVSC_SENSOR_ENDPOINT];
+ sensor->group[SWNODE_IVSC_IPU_PORT] =
+ &nodes[SWNODE_IVSC_IPU_PORT];
+ sensor->group[SWNODE_IVSC_IPU_ENDPOINT] =
+ &nodes[SWNODE_IVSC_IPU_ENDPOINT];
+
+ if (sensor->ssdb.vcmtype)
+ sensor->group[SWNODE_VCM] = &nodes[SWNODE_VCM];
+ } else {
+ if (sensor->ssdb.vcmtype)
+ sensor->group[SWNODE_IVSC_HID] = &nodes[SWNODE_VCM];
+ }
}
static void ipu_bridge_create_connection_swnodes(struct ipu_bridge *bridge,
struct ipu_sensor *sensor)
{
+ struct ipu_node_names *names = &sensor->node_names;
struct software_node *nodes = sensor->swnodes;
char vcm_name[ACPI_ID_LEN + 4];
@@ -238,6 +369,29 @@ static void ipu_bridge_create_connection_swnodes(struct ipu_bridge *bridge,
sensor->node_names.endpoint,
&nodes[SWNODE_IPU_PORT],
sensor->ipu_properties);
+
+ if (sensor->csi_dev) {
+ snprintf(sensor->ivsc_name, sizeof(sensor->ivsc_name), "%s-%u",
+ acpi_device_hid(sensor->ivsc_adev), sensor->ssdb.link);
+
+ nodes[SWNODE_IVSC_HID] = NODE_SENSOR(sensor->ivsc_name,
+ sensor->ivsc_properties);
+ nodes[SWNODE_IVSC_SENSOR_PORT] =
+ NODE_PORT(names->ivsc_sensor_port,
+ &nodes[SWNODE_IVSC_HID]);
+ nodes[SWNODE_IVSC_SENSOR_ENDPOINT] =
+ NODE_ENDPOINT(names->endpoint,
+ &nodes[SWNODE_IVSC_SENSOR_PORT],
+ sensor->ivsc_sensor_ep_properties);
+ nodes[SWNODE_IVSC_IPU_PORT] =
+ NODE_PORT(names->ivsc_ipu_port,
+ &nodes[SWNODE_IVSC_HID]);
+ nodes[SWNODE_IVSC_IPU_ENDPOINT] =
+ NODE_ENDPOINT(names->endpoint,
+ &nodes[SWNODE_IVSC_IPU_PORT],
+ sensor->ivsc_ipu_ep_properties);
+ }
+
if (sensor->ssdb.vcmtype) {
/* append ssdb.link to distinguish VCM nodes with same HID */
snprintf(vcm_name, sizeof(vcm_name), "%s-%u",
@@ -273,6 +427,22 @@ static void ipu_bridge_instantiate_vcm_i2c_client(struct ipu_sensor *sensor)
}
}
+static int ipu_bridge_instantiate_ivsc(struct ipu_sensor *sensor)
+{
+ struct fwnode_handle *fwnode;
+
+ if (!sensor->csi_dev)
+ return 0;
+
+ fwnode = software_node_fwnode(&sensor->swnodes[SWNODE_IVSC_HID]);
+ if (!fwnode)
+ return -ENODEV;
+
+ set_secondary_fwnode(sensor->csi_dev, fwnode);
+
+ return 0;
+}
+
static void ipu_bridge_unregister_sensors(struct ipu_bridge *bridge)
{
struct ipu_sensor *sensor;
@@ -284,6 +454,8 @@ static void ipu_bridge_unregister_sensors(struct ipu_bridge *bridge)
ACPI_FREE(sensor->pld);
acpi_dev_put(sensor->adev);
i2c_unregister_device(sensor->vcm_i2c_client);
+ put_device(sensor->csi_dev);
+ acpi_dev_put(sensor->ivsc_adev);
}
}
@@ -337,12 +509,16 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
goto err_free_pld;
}
+ ret = ipu_bridge_check_ivsc_dev(sensor, adev);
+ if (ret)
+ goto err_free_pld;
+
ipu_bridge_create_fwnode_properties(sensor, bridge, cfg);
ipu_bridge_create_connection_swnodes(bridge, sensor);
ret = software_node_register_node_group(sensor->group);
if (ret)
- goto err_free_pld;
+ goto err_put_dev;
fwnode = software_node_fwnode(&sensor->swnodes[
SWNODE_SENSOR_HID]);
@@ -356,6 +532,10 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
primary = acpi_fwnode_handle(adev);
primary->secondary = fwnode;
+ ret = ipu_bridge_instantiate_ivsc(sensor);
+ if (ret)
+ goto err_free_swnodes;
+
ipu_bridge_instantiate_vcm_i2c_client(sensor);
dev_info(&ipu->dev, "Found supported sensor %s\n",
@@ -368,6 +548,9 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
err_free_swnodes:
software_node_unregister_node_group(sensor->group);
+err_put_dev:
+ put_device(sensor->csi_dev);
+ acpi_dev_put(sensor->ivsc_adev);
err_free_pld:
ACPI_FREE(sensor->pld);
err_put_adev:
diff --git a/drivers/media/pci/intel/ipu-bridge.h b/drivers/media/pci/intel/ipu-bridge.h
index 8cb733c..72ad9db 100644
--- a/drivers/media/pci/intel/ipu-bridge.h
+++ b/drivers/media/pci/intel/ipu-bridge.h
@@ -54,7 +54,12 @@ enum ipu_sensor_swnodes {
SWNODE_SENSOR_ENDPOINT,
SWNODE_IPU_PORT,
SWNODE_IPU_ENDPOINT,
- /* Must be last because it is optional / maybe empty */
+ /* below are optional / maybe empty */
+ SWNODE_IVSC_HID,
+ SWNODE_IVSC_SENSOR_PORT,
+ SWNODE_IVSC_SENSOR_ENDPOINT,
+ SWNODE_IVSC_IPU_PORT,
+ SWNODE_IVSC_IPU_ENDPOINT,
SWNODE_VCM,
SWNODE_COUNT
};
@@ -101,6 +106,8 @@ struct ipu_property_names {
struct ipu_node_names {
char port[7];
+ char ivsc_sensor_port[7];
+ char ivsc_ipu_port[7];
char endpoint[11];
char remote_port[7];
};
@@ -117,6 +124,10 @@ struct ipu_sensor {
struct acpi_device *adev;
struct i2c_client *vcm_i2c_client;
+ struct device *csi_dev;
+ struct acpi_device *ivsc_adev;
+ char ivsc_name[ACPI_ID_LEN + 4];
+
/* SWNODE_COUNT + 1 for terminating NULL */
const struct software_node *group[SWNODE_COUNT + 1];
struct software_node swnodes[SWNODE_COUNT];
@@ -129,9 +140,15 @@ struct ipu_sensor {
struct property_entry ep_properties[5];
struct property_entry dev_properties[5];
struct property_entry ipu_properties[3];
+ struct property_entry ivsc_properties[1];
+ struct property_entry ivsc_sensor_ep_properties[4];
+ struct property_entry ivsc_ipu_ep_properties[4];
+
struct software_node_ref_args local_ref[1];
struct software_node_ref_args remote_ref[1];
struct software_node_ref_args vcm_ref[1];
+ struct software_node_ref_args ivsc_sensor_ref[1];
+ struct software_node_ref_args ivsc_ipu_ref[1];
};
struct ipu_bridge {
--
2.7.4
^ permalink raw reply related
* RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
From: Biju Das @ 2023-06-13 19:31 UTC (permalink / raw)
To: Wolfram Sang, Geert Uytterhoeven, Alexandre Belloni, Mark Brown
Cc: Krzysztof Kozlowski, Laurent Pinchart, Rob Herring, Conor Dooley,
Andrzej Hajda, Neil Armstrong, Robert Foss, David Airlie,
Daniel Vetter, Kieran Bingham, Mauro Carvalho Chehab,
Hans Verkuil, Alessandro Zummo, Alexandre Belloni, Jonas Karlman,
Jernej Skrabec, Uwe Kleine-König, Corey Minyard,
Marek Behún, Jiasheng Jiang, Antonio Borneo, Abhinav Kumar,
Ahmad Fatoum, dri-devel@lists.freedesktop.org,
linux-i2c@vger.kernel.org, linux-media@vger.kernel.org,
Fabrizio Castro, linux-renesas-soc@vger.kernel.org, Mark Brown
In-Reply-To: <OS0PR01MB592210CE54A9CF953980DFEE8655A@OS0PR01MB5922.jpnprd01.prod.outlook.com>
Hi Wolfram,
> Subject: RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
>
> Hi Wolfram,
>
> Thanks for the feedback.
>
> > Subject: RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device
> > API
> >
> > Hi Wolfram,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device
> > > API
> > >
> > > Hi everyone,
> > >
> > > > Perhaps we should first think through what an ancillary device
> > > > really is. My understanding is that it is used to talk to
> > > > secondary addresses of a multi-address I2C slave device.
> > >
> > > As I mentioned somewhere before, this is not the case. Ancillary
> > > devices are when one *driver* handles more than one address.
> > > Everything else has been handled differently in the past (for all
> > > the
> > uses I am aware of).
> > >
> > > Yet, I have another idea which is so simple that I wonder if it
> > > maybe has already been discussed so far?
> > >
> > > * have two regs in the bindings
> >
> > OK, it is inline with DT maintainers expectation as it is matching
> > with real hw as single device node having two regs.
> >
> > > * use the second reg with i2c_new_client_device to instantiate the
> > > RTC sibling. 'struct i2c_board_info', which is one parameter, should
> > > have enough options to pass data, e.g it has a software_node.
> >
> > OK, I can see the below can be passed from PMIC to new client device.
> >
> > client->addr = info->addr;
> >
> > client->init_irq = info->irq;
> >
> > >
> > > Should work or did I miss something here?
> >
> > I guess it will work. We instantiate appropriate device based On PMIC
> > revision and slave address and IRQ resource passed through 'struct
> > i2c_board_info'
> >
> > Will check this and update you.
>
> info.irq = irq; -->Irq fine
> info.addr = addr; -->slave address fine
> size = strscpy(info.type, name, sizeof(info.type)); -->instantiation based
> on PMIC version fine.
>
> 1) How do we share clk details on instantiated device to find is it
> connected to external crystal or external clock source? as we cannot pass
> of_node between PMIC and "i2c_board_info" as it results in pinctrl
> failure. info->platformdata and
> Client->dev.platformdata to retrieve this info??
Or
I2C instantiation based on actual oscillator bit value, ie, two i2c_device_id's
with one for setting oscillator bit and another for clearing oscillator bit
PMIC driver parses the clock details. Based on firmware version and clock,
It instantiates either i2c_device_id with setting oscillator bit or
clearing oscillator bit.
Cheers,
Biju
^ permalink raw reply
* Re: [syzbot] kernel BUG in vmf_insert_pfn_prot
From: syzbot @ 2023-06-13 19:23 UTC (permalink / raw)
To: airlied, airlied, christian.koenig, daniel.vetter, daniel.vetter,
daniel, dri-devel, javierm, linaro-mm-sig-owner, linaro-mm-sig,
linux-kernel, linux-media, maarten.lankhorst, mripard,
suijingfeng, sumit.semwal, syzkaller-bugs, tzimmermann, zackr
In-Reply-To: <000000000000fe7dd005cc2d77c0@google.com>
syzbot suspects this issue was fixed by commit:
commit a5b44c4adb1699661d22e5152fb26885f30a2e4c
Author: Thomas Zimmermann <tzimmermann@suse.de>
Date: Mon Mar 20 15:07:44 2023 +0000
drm/fbdev-generic: Always use shadow buffering
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=1025ee07280000
start commit: 0326074ff465 Merge tag 'net-next-6.1' of git://git.kernel...
git tree: upstream
kernel config: https://syzkaller.appspot.com/x/.config?x=d323d85b1f8a4ed7
dashboard link: https://syzkaller.appspot.com/bug?extid=2d4f8693f438d2bd4bdb
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14fd1182880000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17567514880000
If the result looks correct, please mark the issue as fixed by replying with:
#syz fix: drm/fbdev-generic: Always use shadow buffering
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
^ permalink raw reply
* RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
From: Biju Das @ 2023-06-13 17:57 UTC (permalink / raw)
To: Wolfram Sang, Geert Uytterhoeven
Cc: Krzysztof Kozlowski, Laurent Pinchart, Rob Herring, Conor Dooley,
Andrzej Hajda, Neil Armstrong, Robert Foss, David Airlie,
Daniel Vetter, Kieran Bingham, Mauro Carvalho Chehab,
Hans Verkuil, Alessandro Zummo, Alexandre Belloni, Jonas Karlman,
Jernej Skrabec, Uwe Kleine-König, Corey Minyard,
Marek Behún, Jiasheng Jiang, Antonio Borneo, Abhinav Kumar,
Ahmad Fatoum, dri-devel@lists.freedesktop.org,
linux-i2c@vger.kernel.org, linux-media@vger.kernel.org,
Fabrizio Castro, linux-renesas-soc@vger.kernel.org, Mark Brown
In-Reply-To: <OS0PR01MB592220CCA081848A711D75328655A@OS0PR01MB5922.jpnprd01.prod.outlook.com>
Hi Wolfram,
Thanks for the feedback.
> Subject: RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
>
> Hi Wolfram,
>
> Thanks for the feedback.
>
> > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device
> > API
> >
> > Hi everyone,
> >
> > > Perhaps we should first think through what an ancillary device
> > > really is. My understanding is that it is used to talk to secondary
> > > addresses of a multi-address I2C slave device.
> >
> > As I mentioned somewhere before, this is not the case. Ancillary
> > devices are when one *driver* handles more than one address.
> > Everything else has been handled differently in the past (for all the
> uses I am aware of).
> >
> > Yet, I have another idea which is so simple that I wonder if it maybe
> > has already been discussed so far?
> >
> > * have two regs in the bindings
>
> OK, it is inline with DT maintainers expectation as it is matching with
> real hw as single device node having two regs.
>
> > * use the second reg with i2c_new_client_device to instantiate the
> > RTC sibling. 'struct i2c_board_info', which is one parameter, should
> > have enough options to pass data, e.g it has a software_node.
>
> OK, I can see the below can be passed from PMIC to new client device.
>
> client->addr = info->addr;
>
> client->init_irq = info->irq;
>
> >
> > Should work or did I miss something here?
>
> I guess it will work. We instantiate appropriate device based On PMIC
> revision and slave address and IRQ resource passed through 'struct
> i2c_board_info'
>
> Will check this and update you.
info.irq = irq; -->Irq fine
info.addr = addr; -->slave address fine
size = strscpy(info.type, name, sizeof(info.type)); -->instantiation based on PMIC version fine.
1) How do we share clk details on instantiated device to find is it connected to external crystal or external clock source? as we cannot pass of_node between PMIC and "i2c_board_info" as it results in pinctrl failure. info->platformdata and
Client->dev.platformdata to retrieve this info??
Cheers,
Biju
^ permalink raw reply
* Re: [PATCH v14 1/2] drm: add kms driver for loongson display controller
From: Sui Jingfeng @ 2023-06-13 17:29 UTC (permalink / raw)
To: Sui Jingfeng, WANG Xuerui, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter, Li Yi,
Sumit Semwal, Christian Koenig, Emil Velikov
Cc: linaro-mm-sig, loongson-kernel, Geert Uytterhoeven, linux-kernel,
dri-devel, Javier Martinez Canillas, Nathan Chancellor,
Liu Peibao, linux-media
In-Reply-To: <14e56806-833b-c01b-ee74-8f16f48df2fc@loongson.cn>
On 2023/6/14 00:20, Sui Jingfeng wrote:
> We will remote this workaround at next version.
remote -> remove
^ permalink raw reply
* Re: [PATCH] media: v4l2-core: Fix a potential resource leak in v4l2_fwnode_parse_link()
From: Christophe JAILLET @ 2023-06-13 17:15 UTC (permalink / raw)
To: Sakari Ailus
Cc: Mauro Carvalho Chehab, linux-kernel, kernel-janitors, linux-media
In-Reply-To: <ZIhLDh567eWqY5vk@kekkonen.localdomain>
Le 13/06/2023 à 12:55, Sakari Ailus a écrit :
> Hi Christophe,
>
> On Mon, May 29, 2023 at 08:17:18AM +0200, Christophe JAILLET wrote:
>> 'fwnode is known to be NULL, at this point, so fwnode_handle_put() is a
>> no-op.
>>
>> Release the reference taken from a previous fwnode_graph_get_port_parent()
>> call instead.
>>
>> Fixes: ca50c197bd96 ("[media] v4l: fwnode: Support generic fwnode for parsing standardised properties")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> /!\ THIS PATCH IS SPECULATIVE /!\
>> review with care
>> ---
>> drivers/media/v4l2-core/v4l2-fwnode.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
>> index 049c2f2001ea..b7dd467c53fd 100644
>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
>> @@ -571,7 +571,7 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
>>
>> fwnode = fwnode_graph_get_remote_endpoint(fwnode);
>> if (!fwnode) {
>> - fwnode_handle_put(fwnode);
>> + fwnode_handle_put(link->local_node);
>
> link->local_node also needs to be non-NULL for the successful case. The
> condition should take that into account. Could you send v2 with that?
>
>> return -ENOLINK;
>> }
>>
>
Hi,
something like below?
@@ -568,19 +568,25 @@ int v4l2_fwnode_parse_link(struct fwnode_handle
*fwnode,
link->local_id = fwep.id;
link->local_port = fwep.port;
link->local_node = fwnode_graph_get_port_parent(fwnode);
+ if (!link->local_node)
+ return -ENOLINK;
fwnode = fwnode_graph_get_remote_endpoint(fwnode);
- if (!fwnode) {
- fwnode_handle_put(fwnode);
- return -ENOLINK;
- }
+ if (!fwnode)
+ goto err_put_local_node;
fwnode_graph_parse_endpoint(fwnode, &fwep);
link->remote_id = fwep.id;
link->remote_port = fwep.port;
link->remote_node = fwnode_graph_get_port_parent(fwnode);
+ if (!link->remote_node)
+ goto err_put_local_node;
return 0;
+
+err_put_local_node:
+ fwnode_handle_put(link->local_node);
+ return -ENOLINK;
}
EXPORT_SYMBOL_GPL(v4l2_fwnode_parse_link);
CJ
^ permalink raw reply
* Re: [PATCH 1/1] media: staging: atomisp: select V4L2_FWNODE
From: Hans de Goede @ 2023-06-13 17:08 UTC (permalink / raw)
To: Sakari Ailus, linux-media, Mauro Carvalho Chehab; +Cc: Andy Shevchenko
In-Reply-To: <20230613165109.111837-1-sakari.ailus@linux.intel.com>
Hi,
On 6/13/23 18:51, Sakari Ailus wrote:
> Select V4L2_FWNODE as the driver depends on it.
>
> Reported-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Fixes: aa31f6514047 ("media: atomisp: allow building the driver again")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Mauro this is a build fix for the recent atomisp pull-req
for 6.5, can you merge this please.
Regards,
Hans
> ---
> drivers/staging/media/atomisp/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/staging/media/atomisp/Kconfig b/drivers/staging/media/atomisp/Kconfig
> index c9bff98e5309a..e9b168ba97bf1 100644
> --- a/drivers/staging/media/atomisp/Kconfig
> +++ b/drivers/staging/media/atomisp/Kconfig
> @@ -13,6 +13,7 @@ config VIDEO_ATOMISP
> tristate "Intel Atom Image Signal Processor Driver"
> depends on VIDEO_DEV && INTEL_ATOMISP
> depends on PMIC_OPREGION
> + select V4L2_FWNODE
> select IOSF_MBI
> select VIDEOBUF2_VMALLOC
> select VIDEO_V4L2_SUBDEV_API
^ permalink raw reply
* Re: [PATCH 1/1] media: staging: atomisp: select V4L2_FWNODE
From: Sakari Ailus @ 2023-06-13 17:06 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: linux-media, Hans de Goede
In-Reply-To: <ZIig2sPWRnLvI+iH@smile.fi.intel.com>
Hi Andy,
On Tue, Jun 13, 2023 at 08:01:14PM +0300, Andy Shevchenko wrote:
> On Tue, Jun 13, 2023 at 07:51:09PM +0300, Sakari Ailus wrote:
> > Select V4L2_FWNODE as the driver depends on it.
>
> Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> > Reported-by: Andy Shevchenko <andriy.shevchenko@intel.com>
>
> I believe this should be @linux.intel.com, but it doesn't matter.
I can change that before sending the PR. You used you @intel.com e-mail for
sending it, just FYI.
--
Kind regards,
Sakari Ailus
^ permalink raw reply
* Re: [PATCH 1/1] media: staging: atomisp: select V4L2_FWNODE
From: Andy Shevchenko @ 2023-06-13 17:01 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, Hans de Goede
In-Reply-To: <20230613165109.111837-1-sakari.ailus@linux.intel.com>
On Tue, Jun 13, 2023 at 07:51:09PM +0300, Sakari Ailus wrote:
> Select V4L2_FWNODE as the driver depends on it.
Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reported-by: Andy Shevchenko <andriy.shevchenko@intel.com>
I believe this should be @linux.intel.com, but it doesn't matter.
> Fixes: aa31f6514047 ("media: atomisp: allow building the driver again")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/staging/media/atomisp/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/staging/media/atomisp/Kconfig b/drivers/staging/media/atomisp/Kconfig
> index c9bff98e5309a..e9b168ba97bf1 100644
> --- a/drivers/staging/media/atomisp/Kconfig
> +++ b/drivers/staging/media/atomisp/Kconfig
> @@ -13,6 +13,7 @@ config VIDEO_ATOMISP
> tristate "Intel Atom Image Signal Processor Driver"
> depends on VIDEO_DEV && INTEL_ATOMISP
> depends on PMIC_OPREGION
> + select V4L2_FWNODE
> select IOSF_MBI
> select VIDEOBUF2_VMALLOC
> select VIDEO_V4L2_SUBDEV_API
> --
> 2.30.2
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [RESEND PATCH v3 16/32] media: v4l: async: Drop duplicate handling when adding connections
From: Sakari Ailus @ 2023-06-13 16:58 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Philipp Zabel, hverkuil, Francesco Dolcini,
aishwarya.kothari, Robert Foss, Todor Tomov, Hyun Kwon,
bingbu.cao, niklas.soderlund, Kieran Bingham, Benjamin Mugnier,
Sylvain Petinot, Eugen Hristev, Nicolas Ferre, Maxime Ripard,
Rui Miguel Silva, Fabio Estevam, Bryan O'Donoghue,
Sylwester Nawrocki, Dafna Hirschfeld, Hugues Fruchet, Yong Deng,
Paul Kocialkowski, Lad, Prabhakar, Benoit Parrot,
Steve Longerbeam, Thierry Reding, Jonathan Hunter,
Sowjanya Komatineni, Marco Felsch
In-Reply-To: <20230530060115.GU21633@pendragon.ideasonboard.com>
Hi Laurent,
On Tue, May 30, 2023 at 09:01:15AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Thu, May 25, 2023 at 12:15:59PM +0300, Sakari Ailus wrote:
> > The connections are checked for duplicates already when the notifier is
> > registered. This is effectively a sanity check for driver (and possibly
> > obscure firmware) bugs. Don't do this when adding the connection.
>
> Isn't it better to have this sanity check when the connection is added,
> instead of later when the notifier is registered ? The latter is more
> difficult to debug. If you want to avoid duplicate checks, could we drop
> the one at notifier registration time ?
I've never seen or heard this check failing. I'm therefore not very
concerned keeping it as easy to debug as possible, instead I prefer simpler
implementation.
Checking at the registration time is still necessary as the same match
could have been added to another notifier while this one was being set up.
--
Kind regards,
Sakari Ailus
^ 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