* Re: [PATCH v3 2/2] mailbox: arm_mhuv3: Add driver
From: Jonathan Cameron @ 2024-04-05 10:32 UTC (permalink / raw)
To: Cristian Marussi
Cc: linux-kernel, linux-arm-kernel, devicetree, sudeep.holla,
jassisinghbrar, robh+dt, krzysztof.kozlowski+dt, conor+dt
In-Reply-To: <20240404062347.3219795-3-cristian.marussi@arm.com>
On Thu, 4 Apr 2024 07:23:47 +0100
Cristian Marussi <cristian.marussi@arm.com> wrote:
> Add support for ARM MHUv3 mailbox controller.
>
> Support is limited to the MHUv3 Doorbell extension using only the PBX/MBX
> combined interrupts.
>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
Drive by review (I was curious what this was :)
> diff --git a/drivers/mailbox/arm_mhuv3.c b/drivers/mailbox/arm_mhuv3.c
> new file mode 100644
> index 000000000000..e4125568bec0
> --- /dev/null
> +++ b/drivers/mailbox/arm_mhuv3.c
> @@ -0,0 +1,1063 @@
> +struct dummy_page {
> + u8 pad[0x1000];
> +} __packed;
> +
> +struct mhu3_pbx_frame_reg {
> + struct ctrl_page ctrl;
> + struct pdbcw_page dbcw[MHUV3_DBCW_MAX];
> + struct dummy_page ffcw;
> + struct dummy_page fcw;
> + u8 pad[0xF000 - 0x4000];
> + struct dummy_page impdef;
> +} __packed;
> +
> +struct mhu3_mbx_frame_reg {
> + struct ctrl_page ctrl;
> + struct mdbcw_page dbcw[MHUV3_DBCW_MAX];
> + struct dummy_page ffcw;
> + struct dummy_page fcw;
> + u8 pad[0xF000 - 0x4000];
Magic, numbers, Maybe give them a definition or base them on something
meaningful such as structure offsets?
> + struct dummy_page impdef;
> +} __packed;
> +
> +/* Macro for reading a bitfield within a physically mapped packed struct */
> +#define readl_relaxed_bitfield(_regptr, _field) \
> + ({ \
> + u32 _rval; \
> + typeof(_regptr) _rptr = _regptr; \
> + _rval = readl_relaxed(_rptr); \
> + ((typeof(*_rptr) __force *)(&_rval))->_field; \
> + })
> +
> +/* Macro for writing a bitfield within a physically mapped packed struct */
> +#define writel_relaxed_bitfield(_value, _regptr, _field) \
> + ({ \
> + u32 _rval; \
> + typeof(_regptr) _rptr = _regptr; \
> + _rval = readl_relaxed(_rptr); \
> + ((typeof(*_rptr) __force *)(&_rval))->_field = _value; \
> + writel_relaxed(_rval, _rptr); \
> + })
Similar, yet slightly different from ones in arm_mhuv2.c? Why the differences
and can these be shared code in a suitable header?
> +
> +/* ====== MHUv3 data structures ====== */
> +
> +enum mhuv3_frame {
> + PBX_FRAME,
> + MBX_FRAME
Trailing commas for last entries in enums unless they are in some sense terminators.
> +};
> +
> +static char *mhuv3_str[] = {
> + "PBX",
> + "MBX"
> +};
> +
> +enum mhuv3_extension_type {
> + FIRST_EXT = 0,
As mentioned inline, 0 is kind of default assumption for first so I wouldn't define it.
> + DBE_EXT = FIRST_EXT,
> + FCE_EXT,
> + FE_EXT,
> + MAX_EXT
That's one past normal meeting of MAX, maybe call it COUNT, or NUM?
> +};
> +static int mhuv3_doorbell_send_data(struct mhuv3 *mhu, struct mbox_chan *chan,
> + void *arg)
> +{
> + int ret = 0;
> + struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
> + struct mhuv3_extension *e = mhu->ext[DBE_EXT];
> + unsigned long flags;
> +
> + spin_lock_irqsave(&e->pending_lock, flags);
guard() then you can do earlier returns and end up with cleaner code.
> + /* Only one in-flight Transfer is allowed per-doorbell */
> + if (!(e->pending_db[priv->ch_idx] & BIT(priv->doorbell))) {
> + e->pending_db[priv->ch_idx] |= BIT(priv->doorbell);
> + writel_relaxed(BIT(priv->doorbell),
> + &mhu->pbx->dbcw[priv->ch_idx].set);
> + } else {
> + ret = -EBUSY;
> + }
> + spin_unlock_irqrestore(&e->pending_lock, flags);
> +
> + return ret;
> +}
>
> +
> +static struct mbox_chan *mhuv3_dbe_chan_from_comb_irq_get(struct mhuv3 *mhu)
> +{
> + int i;
> + struct mhuv3_extension *e = mhu->ext[DBE_EXT];
> + struct device *dev = mhu->mbox.dev;
> +
> + for (i = 0; i < MHUV3_DBCH_CMB_INT_ST_REG_CNT; i++) {
> + unsigned int channel, db = MHUV3_INVALID_DOORBELL;
> + u32 cmb_st, st;
> +
> + cmb_st = readl_relaxed(&mhu->ctrl->dbch_int_st[i]);
> + if (!cmb_st)
> + continue;
> +
> + channel = i * MHUV3_STAT_BITS + __builtin_ctz(cmb_st);
> + if (channel >= e->max_chans) {
> + dev_err(dev, "Invalid %s channel:%d\n",
> + mhuv3_str[mhu->frame], channel);
return here rather than breaking out the loop. It is easier to follow
given nothing is done after the loop
> + break;
> + }
> +
> + if (mhu->frame == PBX_FRAME) {
> + unsigned long flags;
> + u32 active_dbs, fired_dbs;
> +
> + st = readl_relaxed_bitfield(&mhu->pbx->dbcw[channel].int_st,
> + tfr_ack);
> + if (!st) {
> + dev_warn(dev, "Spurios IRQ on %s channel:%d\n",
Spell check. Spurious.
> + mhuv3_str[mhu->frame], channel);
> + continue;
> + }
> +
> + active_dbs = readl_relaxed(&mhu->pbx->dbcw[channel].st);
> + spin_lock_irqsave(&e->pending_lock, flags);
> + fired_dbs = e->pending_db[channel] & ~active_dbs;
> + if (fired_dbs) {
> + db = __builtin_ctz(fired_dbs);
> + e->pending_db[channel] &= ~BIT(db);
> + fired_dbs &= ~BIT(db);
> + }
> + spin_unlock_irqrestore(&e->pending_lock, flags);
> +
> + /* Clear TFR Ack if no more doorbells pending */
> + if (!fired_dbs)
> + writel_relaxed_bitfield(0x1,
> + &mhu->pbx->dbcw[channel].int_clr,
> + tfr_ack);
> + } else {
> + st = readl_relaxed(&mhu->mbx->dbcw[channel].st_msk);
> + if (!st) {
> + dev_warn(dev, "Spurios IRQ on %s channel:%d\n",
> + mhuv3_str[mhu->frame], channel);
> + continue;
> + }
> + db = __builtin_ctz(st);
> + }
> +
> + if (db != MHUV3_INVALID_DOORBELL) {
> + dev_dbg(dev, "Found %s ch[%d]/db[%d]\n",
> + mhuv3_str[mhu->frame], channel, db);
> +
> + return &mhu->mbox.chans[channel * MHUV3_STAT_BITS + db];
> + }
> + }
> +
> + return ERR_PTR(-EIO);
> +}
> +
> +static int mhuv3_dbe_init(struct mhuv3 *mhu)
> +{
> + struct mhuv3_extension *e;
> + struct device *dev = mhu->mbox.dev;
> +
> + if (!readl_relaxed_bitfield(&mhu->ctrl->feat_spt0, dbe_spt))
> + return 0;
> +
> + dev_dbg(dev, "%s: Initializing DBE Extension.\n", mhuv3_str[mhu->frame]);
> +
> + e = devm_kzalloc(dev, sizeof(*e), GFP_KERNEL);
> + if (!e)
> + return -ENOMEM;
> +
> + e->type = DBE_EXT;
> + /* Note that, by the spec, the number of channels is (num_dbch + 1) */
> + e->max_chans =
> + readl_relaxed_bitfield(&mhu->ctrl->dbch_cfg0, num_dbch) + 1;
> + e->mbox_of_xlate = mhuv3_dbe_mbox_of_xlate;
> + e->combined_irq_setup = mhuv3_dbe_combined_irq_setup;
> + e->channels_init = mhuv3_dbe_channels_init;
> + e->chan_from_comb_irq_get = mhuv3_dbe_chan_from_comb_irq_get;
> +
> + mhu->tot_chans += e->max_chans * MHUV3_STAT_BITS;
> + mhu->ext[DBE_EXT] = e;
> +
> + dev_info(dev, "%s: found %d DBE channels.\n",
> + mhuv3_str[mhu->frame], e->max_chans);
dev_dbg() probably more appropriate.
> +
> + return 0;
> +}
> +
> +static int mhuv3_fce_init(struct mhuv3 *mhu)
> +{
> + struct device *dev = mhu->mbox.dev;
> +
> + if (!readl_relaxed_bitfield(&mhu->ctrl->feat_spt0, fce_spt))
> + return 0;
> +
> + dev_dbg(dev, "%s: FCE Extension not supported by driver.\n",
> + mhuv3_str[mhu->frame]);
> +
> + return 0;
> +}
> +
> +static int mhuv3_fe_init(struct mhuv3 *mhu)
> +{
> + struct device *dev = mhu->mbox.dev;
> +
> + if (!readl_relaxed_bitfield(&mhu->ctrl->feat_spt0, fe_spt))
> + return 0;
> +
> + dev_dbg(dev, "%s: FE Extension not supported by driver.\n",
> + mhuv3_str[mhu->frame]);
> +
> + return 0;
> +}
> +
> +static mhuv3_extension_initializer mhuv3_extension_init[MAX_EXT] = {
> + mhuv3_dbe_init,
> + mhuv3_fce_init,
> + mhuv3_fe_init,
> +};
> +
> +static int mhuv3_initialize_channels(struct device *dev, struct mhuv3 *mhu)
> +{
> + int i, ret = 0;
> + struct mbox_controller *mbox = &mhu->mbox;
> +
> + mbox->chans = devm_kcalloc(dev, mhu->tot_chans,
> + sizeof(*mbox->chans), GFP_KERNEL);
> + if (!mbox->chans)
> + return -ENOMEM;
> +
> + for (i = FIRST_EXT; i < MAX_EXT && !ret; i++)
Why this dance with FIRST_EXT if it is always 0? Cleaner to just use 0.
> + if (mhu->ext[i])
> + ret = mhu->ext[i]->channels_init(mhu);
> +
> + return ret;
> +}
> +
> +static struct mbox_chan *mhuv3_mbox_of_xlate(struct mbox_controller *mbox,
> + const struct of_phandle_args *pa)
> +{
> + unsigned int type, channel, param;
> + struct mhuv3 *mhu = mhu_from_mbox(mbox);
> +
> + if (pa->args_count != MHUV3_MBOX_CELLS)
> + return ERR_PTR(-EINVAL);
> +
> + type = pa->args[MHUV3_MBOX_CELL_TYPE];
> + if (type >= MAX_EXT)
> + return ERR_PTR(-EINVAL);
> +
> + channel = pa->args[MHUV3_MBOX_CELL_CHWN];
> + param = pa->args[MHUV3_MBOX_CELL_PARAM];
> +
> + return mhu->ext[type]->mbox_of_xlate(mhu, channel, param);
> +}
> +
> +static int mhuv3_frame_init(struct mhuv3 *mhu, void __iomem *regs)
> +{
> + int i, ret = 0;
> + struct device *dev = mhu->mbox.dev;
> +
> + mhu->ctrl = regs;
> + mhu->frame = readl_relaxed_bitfield(&mhu->ctrl->blk_id, blk_id);
> + if (mhu->frame > MBX_FRAME) {
> + dev_err(dev, "Invalid Frame type- %d\n", mhu->frame);
> + return -EINVAL;
dev_err_probe() etc (see later)
> + }
> +
> + mhu->major = readl_relaxed_bitfield(&mhu->ctrl->aidr, arch_major_rev);
> + mhu->minor = readl_relaxed_bitfield(&mhu->ctrl->aidr, arch_minor_rev);
> + if (mhu->major != MHUV3_MAJOR_VERSION) {
> + dev_warn(dev, "Unsupported MHU %s block - major:%d minor:%d\n",
> + mhuv3_str[mhu->frame], mhu->major, mhu->minor);
You are treating it as an error, so why only a warning?
> + return -EINVAL;
> + }
> + mhu->auto_op_full = !!readl_relaxed_bitfield(&mhu->ctrl->feat_spt1,
> + auto_op_spt);
> + /* Request the PBX/MBX to remain operational */
> + if (mhu->auto_op_full)
> + writel_relaxed_bitfield(0x1, &mhu->ctrl->ctrl, op_req);
> +
> + dev_dbg(dev, "Found MHU %s block - major:%d minor:%d\n",
> + mhuv3_str[mhu->frame], mhu->major, mhu->minor);
> +
> + if (mhu->frame == PBX_FRAME)
> + mhu->pbx = regs;
> + else
> + mhu->mbx = regs;
> +
> + for (i = FIRST_EXT; i < MAX_EXT && !ret; i++)
> + ret = mhuv3_extension_init[i](mhu);
Only dbe_init() returns any errors, so if I ready this correctly you always
eat that error.
> +
> + return ret;
> +}
> +
> +static irqreturn_t mhuv3_pbx_comb_interrupt(int irq, void *arg)
> +{
> + int ret = IRQ_NONE;
> + unsigned int i, found = 0;
> + struct mhuv3 *mhu = arg;
> + struct device *dev = mhu->mbox.dev;
> + struct mbox_chan *chan;
> +
> + for (i = FIRST_EXT; i < MAX_EXT; i++) {
> + /* FCE does not participate to the PBX combined */
> + if (i == FCE_EXT || !mhu->ext[i])
> + continue;
> +
> + chan = mhu->ext[i]->chan_from_comb_irq_get(mhu);
> + if (!IS_ERR(chan)) {
if (IS_ERR(chan))
continue;
will reduce indent and give more readable code.
> + struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
> +
> + found++;
> + if (chan->cl) {
> + mbox_chan_txdone(chan, 0);
> + ret = IRQ_HANDLED;
> + } else {
> + dev_warn(dev,
> + "TX Ack on UNBOUND channel (%u)\n",
> + priv->ch_idx);
> + }
> + }
> + }
> +
> + if (!found)
> + dev_warn_once(dev, "Failed to find channel for the TX interrupt\n");
> +
> + return ret;
> +}
> +
> +static irqreturn_t mhuv3_mbx_comb_interrupt(int irq, void *arg)
> +{
> + int ret = IRQ_NONE;
> + unsigned int i, found = 0;
> + struct mhuv3 *mhu = arg;
> + struct device *dev = mhu->mbox.dev;
> + struct mbox_chan *chan;
> +
> + for (i = FIRST_EXT; i < MAX_EXT; i++) {
> + if (!mhu->ext[i])
> + continue;
> +
> + /* Process any extension which could be source of the IRQ */
> + chan = mhu->ext[i]->chan_from_comb_irq_get(mhu);
> + if (!IS_ERR(chan)) {
if (IS_ERR(chan))
continue;
is going to be easier to read.
> + void *data = NULL;
> + struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
> +
> + found++;
> + /* Read and acknowledge optional in-band LE data first. */
> + if (priv->ops->read_data)
> + data = priv->ops->read_data(mhu, chan);
> +
> + if (chan->cl && !IS_ERR(data)) {
> + mbox_chan_received_data(chan, data);
> + ret = IRQ_HANDLED;
> + } else if (!chan->cl) {
> + dev_warn(dev,
> + "RX Data on UNBOUND channel (%u)\n",
> + priv->ch_idx);
> + } else {
> + dev_err(dev, "Failed to read data: %lu\n",
> + PTR_ERR(data));
> + }
I'd be tempted to factor out this code block into another function as I think
that will allow you to deal with the errors more directly.
> +
> + if (!IS_ERR(data))
> + kfree(data);
> +
> + /*
> + * Acknowledge transfer after any possible optional
> + * out-of-band data has also been retrieved via
> + * mbox_chan_received_data().
> + */
> + if (priv->ops->rx_complete)
> + priv->ops->rx_complete(mhu, chan);
> + }
> + }
> +
> + if (!found)
> + dev_warn_once(dev, "Failed to find channel for the RX interrupt\n");
> +
> + return ret;
> +}
> +
> +static int mhuv3_setup_pbx(struct mhuv3 *mhu)
> +{
> + struct device *dev = mhu->mbox.dev;
> +
> + mhu->mbox.ops = &mhuv3_sender_ops;
> +
> + if (mhu->cmb_irq > 0) {
> + int ret;
> +
> + ret = devm_request_threaded_irq(dev, mhu->cmb_irq, NULL,
> + mhuv3_pbx_comb_interrupt,
> + IRQF_ONESHOT, "mhuv3-pbx", mhu);
> + if (!ret) {
> + int i;
> +
> + mhu->mbox.txdone_irq = true;
> + mhu->mbox.txdone_poll = false;
> +
> + for (i = FIRST_EXT; i < MAX_EXT; i++)
> + if (mhu->ext[i])
> + mhu->ext[i]->combined_irq_setup(mhu);
> +
> + dev_dbg(dev, "MHUv3 PBX IRQs initialized.\n");
> +
> + return 0;
> + }
> +
> + dev_err(dev, "Failed to request PBX IRQ - ret:%d", ret);
If an irq was provided and it failed, I'd just return an error, not muddle on.
Broken system. If it's not an 'error' then don't use dev_err()
Papering over this leads to an odd code flow with if (!ret) so it would
be nice not to bother unless there is a strong reason to carry on.
> + }
> +
> + dev_info(dev, "Using PBX in Tx polling mode.\n");
That's noisy. dev_dbg() perhaps?
> + mhu->mbox.txdone_irq = false;
> + mhu->mbox.txdone_poll = true;
> + mhu->mbox.txpoll_period = 1;
> +
> + return 0;
> +}
> +
> +static int mhuv3_setup_mbx(struct mhuv3 *mhu)
> +{
> + int ret, i;
> + struct device *dev = mhu->mbox.dev;
> +
> + mhu->mbox.ops = &mhuv3_receiver_ops;
> +
> + if (mhu->cmb_irq <= 0) {
> + dev_err(dev, "Missing MBX combined IRQ !\n");
return dev_err_probe()
here as I think it's only called form init. Sure you might not
need the deferred handling it provides but it still leads to
cleaner code and no one has to think about whether deferal might
happen or not.
> + return -EINVAL;
> + }
> +
> + ret = devm_request_threaded_irq(dev, mhu->cmb_irq, NULL,
> + mhuv3_mbx_comb_interrupt, IRQF_ONESHOT,
> + "mhuv3-mbx", mhu);
> + if (ret) {
> + dev_err(dev, "Failed to request MBX IRQ - ret:%d\n", ret);
> + return ret;
return dev_err_probe()
> + }
> +
> + for (i = FIRST_EXT; i < MAX_EXT; i++)
> + if (mhu->ext[i])
> + mhu->ext[i]->combined_irq_setup(mhu);
> +
> + dev_dbg(dev, "MHUv3 MBX IRQs initialized.\n");
> +
> + return ret;
> +}
> +
> +static int mhuv3_irqs_init(struct mhuv3 *mhu, struct platform_device *pdev)
> +{
> + int ret;
> +
> + dev_dbg(mhu->mbox.dev, "Initializing %s block.\n", mhuv3_str[mhu->frame]);
> +
> + if (mhu->frame == PBX_FRAME) {
> + mhu->cmb_irq = platform_get_irq_byname_optional(pdev, "combined");
> + ret = mhuv3_setup_pbx(mhu);
return early is both shorter and easier to follow if people
are looking at particular paths through the function.
> + } else {
> + mhu->cmb_irq = platform_get_irq_byname(pdev, "combined");
> + ret = mhuv3_setup_mbx(mhu);
> + }
> +
> + return ret;
> +}
> +
> +static int mhuv3_probe(struct platform_device *pdev)
> +{
> + int ret;
> + struct mhuv3 *mhu;
> + void __iomem *regs;
> + struct device *dev = &pdev->dev;
> +
> + mhu = devm_kzalloc(dev, sizeof(*mhu), GFP_KERNEL);
> + if (!mhu)
> + return -ENOMEM;
> +
> + regs = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(regs))
> + return PTR_ERR(regs);
> +
> + mhu->mbox.dev = dev;
> + ret = mhuv3_frame_init(mhu, regs);
> + if (ret)
> + return ret;
> +
> + ret = mhuv3_irqs_init(mhu, pdev);
> + if (ret)
> + return ret;
> +
> + mhu->mbox.of_xlate = mhuv3_mbox_of_xlate;
> + ret = mhuv3_initialize_channels(dev, mhu);
> + if (ret)
> + return ret;
> +
> + ret = devm_mbox_controller_register(dev, &mhu->mbox);
> + if (ret)
> + dev_err(dev, "failed to register ARM MHUv3 driver %d\n", ret);
Use dev_err_probe() to get a few things for free in probe time error messages message.
return dev_err_probe(dev, reg, "failed to register ARM HMUv3 driver\n");
return 0;
> +
> + platform_set_drvdata(pdev, mhu);
With all devm as suggested below, can I think drop this.
> +
> + return ret;
> +}
> +
> +static int mhuv3_remove(struct platform_device *pdev)
> +{
> + struct mhuv3 *mhu = platform_get_drvdata(pdev);
> +
> + if (mhu->auto_op_full)
> + writel_relaxed_bitfield(0x0, &mhu->ctrl->ctrl, op_req);
> +
From a quick glance probably better to use a
devm_add_action_or_reset() so that this is turned off at
equivalent place in remove() path as where it is turned on in _init()
Only register the callback if auto_op_full()
Mixing and matching devm_ and calls in remove is a path to weird
races and corner cases so better to go all in on devm handling.
> + return 0;
> +}
> +
> +static const struct of_device_id mhuv3_of_match[] = {
> + { .compatible = "arm,mhuv3", .data = NULL },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, mhuv3_of_match);
> +
> +static struct platform_driver mhuv3_driver = {
> + .driver = {
> + .name = "arm-mhuv3-mailbox",
> + .of_match_table = mhuv3_of_match,
> + },
> + .probe = mhuv3_probe,
> + .remove = mhuv3_remove,
> +};
> +module_platform_driver(mhuv3_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("ARM MHUv3 Driver");
> +MODULE_AUTHOR("Cristian Marussi <cristian.marussi@arm.com>");
^ permalink raw reply
* Re: [PATCH v3 21/25] drivers: media: i2c: imx258: Use macros
From: Luis Garcia @ 2024-04-05 10:33 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, dave.stevenson, jacopo.mondi, mchehab, robh,
krzysztof.kozlowski+dt, conor+dt, shawnguo, s.hauer, kernel,
festevam, devicetree, imx, linux-arm-kernel, linux-kernel, pavel,
phone-devel, Ondrej Jirman
In-Reply-To: <Zg5Mz0QSqNDXzY4o@kekkonen.localdomain>
On 4/4/24 00:46, Sakari Ailus wrote:
> On Wed, Apr 03, 2024 at 01:17:26PM -0600, Luigi311 wrote:
>> On 4/3/24 10:23, Sakari Ailus wrote:
>>> Hi Luis,
>>>
>>> On Wed, Apr 03, 2024 at 09:03:50AM -0600, git@luigi311.com wrote:
>>>> From: Luis Garcia <git@luigi311.com>
>>>>
>>>> Use understandable macros instead of raw values.
>>>>
>>>> Signed-off-by: Ondrej Jirman <megi@xff.cz>
>>>> Signed-off-by: Luis Garcia <git@luigi311.com>
>>>> ---
>>>> drivers/media/i2c/imx258.c | 434 ++++++++++++++++++-------------------
>>>> 1 file changed, 207 insertions(+), 227 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
>>>> index e2ecf6109516..30352c33f63c 100644
>>>> --- a/drivers/media/i2c/imx258.c
>>>> +++ b/drivers/media/i2c/imx258.c
>>>> @@ -33,8 +33,6 @@
>>>> #define IMX258_VTS_30FPS_VGA 0x034c
>>>> #define IMX258_VTS_MAX 65525
>>>>
>>>> -#define IMX258_REG_VTS 0x0340
>>>> -
>>>> /* HBLANK control - read only */
>>>> #define IMX258_PPL_DEFAULT 5352
>>>>
>>>> @@ -90,6 +88,53 @@
>>>> #define IMX258_PIXEL_ARRAY_WIDTH 4208U
>>>> #define IMX258_PIXEL_ARRAY_HEIGHT 3120U
>>>>
>>>> +/* regs */
>>>> +#define IMX258_REG_PLL_MULT_DRIV 0x0310
>>>> +#define IMX258_REG_IVTPXCK_DIV 0x0301
>>>> +#define IMX258_REG_IVTSYCK_DIV 0x0303
>>>> +#define IMX258_REG_PREPLLCK_VT_DIV 0x0305
>>>> +#define IMX258_REG_IOPPXCK_DIV 0x0309
>>>> +#define IMX258_REG_IOPSYCK_DIV 0x030b
>>>> +#define IMX258_REG_PREPLLCK_OP_DIV 0x030d
>>>> +#define IMX258_REG_PHASE_PIX_OUTEN 0x3030
>>>> +#define IMX258_REG_PDPIX_DATA_RATE 0x3032
>>>> +#define IMX258_REG_SCALE_MODE 0x0401
>>>> +#define IMX258_REG_SCALE_MODE_EXT 0x3038
>>>> +#define IMX258_REG_AF_WINDOW_MODE 0x7bcd
>>>> +#define IMX258_REG_FRM_LENGTH_CTL 0x0350
>>>> +#define IMX258_REG_CSI_LANE_MODE 0x0114
>>>> +#define IMX258_REG_X_EVN_INC 0x0381
>>>> +#define IMX258_REG_X_ODD_INC 0x0383
>>>> +#define IMX258_REG_Y_EVN_INC 0x0385
>>>> +#define IMX258_REG_Y_ODD_INC 0x0387
>>>> +#define IMX258_REG_BINNING_MODE 0x0900
>>>> +#define IMX258_REG_BINNING_TYPE_V 0x0901
>>>> +#define IMX258_REG_FORCE_FD_SUM 0x300d
>>>> +#define IMX258_REG_DIG_CROP_X_OFFSET 0x0408
>>>> +#define IMX258_REG_DIG_CROP_Y_OFFSET 0x040a
>>>> +#define IMX258_REG_DIG_CROP_IMAGE_WIDTH 0x040c
>>>> +#define IMX258_REG_DIG_CROP_IMAGE_HEIGHT 0x040e
>>>> +#define IMX258_REG_SCALE_M 0x0404
>>>> +#define IMX258_REG_X_OUT_SIZE 0x034c
>>>> +#define IMX258_REG_Y_OUT_SIZE 0x034e
>>>> +#define IMX258_REG_X_ADD_STA 0x0344
>>>> +#define IMX258_REG_Y_ADD_STA 0x0346
>>>> +#define IMX258_REG_X_ADD_END 0x0348
>>>> +#define IMX258_REG_Y_ADD_END 0x034a
>>>> +#define IMX258_REG_EXCK_FREQ 0x0136
>>>> +#define IMX258_REG_CSI_DT_FMT 0x0112
>>>> +#define IMX258_REG_LINE_LENGTH_PCK 0x0342
>>>> +#define IMX258_REG_SCALE_M_EXT 0x303a
>>>> +#define IMX258_REG_FRM_LENGTH_LINES 0x0340
>>>> +#define IMX258_REG_FINE_INTEG_TIME 0x0200
>>>> +#define IMX258_REG_PLL_IVT_MPY 0x0306
>>>> +#define IMX258_REG_PLL_IOP_MPY 0x030e
>>>> +#define IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H 0x0820
>>>> +#define IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L 0x0822
>>>> +
>>>> +#define REG8(a, v) { a, v }
>>>> +#define REG16(a, v) { a, ((v) >> 8) & 0xff }, { (a) + 1, (v) & 0xff }
>>>
>>> The patch is nice but these macros are better replaced by the V4L2 CCI
>>> helper that also offers register access functions. Could you add a patch to
>>> convert the driver to use it (maybe after this one)?
>>>
>>
>> Ohh perfect, using something else would be great. Ill go ahead and see
>> if I can get that working.
>
> Thanks. It may be easier to just do it in this one actually. Up to you.
>
I've made the swap but looks like its not playing nice with my ppp,
its causing a crash and showing a call trace as soon as it does its
first read to check the identity. I went in and dropped the cci_read
and left it with the original implementation and I'm getting a very
similar crash with cci_write too so it looks like its not liking
how I'm implementing it. Looking at the few other drivers that were
swapped over to use that, I don't seem to be missing anything. It's
a big change so its not really something I can describe what I've
changed but I do have the change on my github here
https://github.com/luigi311/linux/commit/840593acb20eee87ce361e6929edf51eefbbe737
if you can provide some guidance, if not I can skip this change
all together and we can do a separate attempt at swapping over to it.
^ permalink raw reply
* Re: [PATCH v2 1/2] ASoC: dt-bindings: qcom,sm8250: Add QCM6490 snd QCS6490 sound card
From: Dmitry Baryshkov @ 2024-04-05 10:46 UTC (permalink / raw)
To: Mohammad Rafi Shaik
Cc: Srinivas Kandagatla, Banajit Goswami, Liam Girdwood, Mark Brown,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jaroslav Kysela,
Takashi Iwai, linux-arm-msm, alsa-devel, linux-sound, devicetree,
linux-kernel, quic_rohkumar
In-Reply-To: <797d67b9-9e09-8b84-9abc-dd4a4a2a40f5@quicinc.com>
On Fri, 5 Apr 2024 at 08:56, Mohammad Rafi Shaik <quic_mohs@quicinc.com> wrote:
>
> On 4/4/2024 2:23 PM, Dmitry Baryshkov wrote:
> > On Thu, 4 Apr 2024 at 11:48, Mohammad Rafi Shaik <quic_mohs@quicinc.com> wrote:
> >>
> >> Document the bindings for the Qualcomm QCM6490 IDP and QCS6490 RB3Gen2
> >> soc platforms sound card.
> >>
> >> The bindings are the same as for other newer Qualcomm ADSP sound cards,
> >> thus keep them in existing qcom,sm8250.yaml file, even though Linux driver
> >> is separate.
> >>
> >> Signed-off-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com>
> >> ---
> >> Documentation/devicetree/bindings/sound/qcom,sm8250.yaml | 2 ++
> >> 1 file changed, 2 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml b/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml
> >> index 2ab6871e89e5..ff1a27f26bc2 100644
> >> --- a/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml
> >> +++ b/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml
> >> @@ -29,6 +29,8 @@ properties:
> >> - enum:
> >> - qcom,apq8016-sbc-sndcard
> >> - qcom,msm8916-qdsp6-sndcard
> >> + - qcom,qcm6490-sndcard
> >> + - qcom,qcs6490-rb3gen2-sndcard
> >
> > My 2c: you are adding one soundcard for the SoC family (qcm6490) and
> > another one for the particular board kind (qcs6490-rb3gen2). That
> > doesn't seem logical.
>
> The qcm6490-sndcard compatible for enabling soundcard on
> qcm6490 IDP boards.
>
> Will change compatible name as qcom,qcm6490-idp-sndcard.
Any consistent way is fine with me.
--
With best wishes
Dmitry
^ permalink raw reply
* [PATCH] arm64: dts: mediatek: mt7981: fix code alignment for PWM clocks
From: Rafał Miłecki @ 2024-04-05 10:50 UTC (permalink / raw)
To: Matthias Brugger, AngeloGioacchino Del Regno, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: devicetree, linux-arm-kernel, linux-mediatek, linux-kernel,
Rafał Miłecki
From: Rafał Miłecki <rafal@milecki.pl>
Align "clocks" array entries to start at the same column.
Fixes: cf29427573cc ("arm64: dts: mediatek: Add initial MT7981B and Xiaomi AX3000T")
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
arch/arm64/boot/dts/mediatek/mt7981b.dtsi | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/boot/dts/mediatek/mt7981b.dtsi b/arch/arm64/boot/dts/mediatek/mt7981b.dtsi
index 5674ac81d1f8..8a6263cc569c 100644
--- a/arch/arm64/boot/dts/mediatek/mt7981b.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt7981b.dtsi
@@ -86,10 +86,10 @@ pwm@10048000 {
compatible = "mediatek,mt7981-pwm";
reg = <0 0x10048000 0 0x1000>;
clocks = <&infracfg CLK_INFRA_PWM_STA>,
- <&infracfg CLK_INFRA_PWM_HCK>,
- <&infracfg CLK_INFRA_PWM1_CK>,
- <&infracfg CLK_INFRA_PWM2_CK>,
- <&infracfg CLK_INFRA_PWM3_CK>;
+ <&infracfg CLK_INFRA_PWM_HCK>,
+ <&infracfg CLK_INFRA_PWM1_CK>,
+ <&infracfg CLK_INFRA_PWM2_CK>,
+ <&infracfg CLK_INFRA_PWM3_CK>;
clock-names = "top", "main", "pwm1", "pwm2", "pwm3";
#pwm-cells = <2>;
};
--
2.35.3
^ permalink raw reply related
* Re: [PATCH v3 19/25] media: i2c: imx258: Change register settings for variants of the sensor
From: Sakari Ailus @ 2024-04-05 10:59 UTC (permalink / raw)
To: Luigi311
Cc: linux-media, dave.stevenson, jacopo.mondi, mchehab, robh,
krzysztof.kozlowski+dt, conor+dt, shawnguo, s.hauer, kernel,
festevam, devicetree, imx, linux-arm-kernel, linux-kernel, pavel,
phone-devel
In-Reply-To: <998efafa-699b-4226-91d4-2ebba85d63ec@luigi311.com>
Hi Luis, Dave,
On Thu, Apr 04, 2024 at 04:44:05PM -0600, Luigi311 wrote:
> On 4/3/24 10:18, Sakari Ailus wrote:
> > Hi Luis, Dave,
> >
> > On Wed, Apr 03, 2024 at 09:03:48AM -0600, git@luigi311.com wrote:
> >> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >>
> >> Sony have advised that there are variants of the IMX258 sensor which
> >> require slightly different register configuration to the mainline
> >> imx258 driver defaults.
> >>
> >> There is no available run-time detection for the variant, so add
> >> configuration via the DT compatible string.
> >>
> >> The Vision Components imx258 module supports PDAF, so add the
> >> register differences for that variant
> >>
> >> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >> Signed-off-by: Luis Garcia <git@luigi311.com>
> >> ---
> >> drivers/media/i2c/imx258.c | 48 ++++++++++++++++++++++++++++++++++----
> >> 1 file changed, 44 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> >> index 775d957c9b87..fa48da212037 100644
> >> --- a/drivers/media/i2c/imx258.c
> >> +++ b/drivers/media/i2c/imx258.c
> >> @@ -6,6 +6,7 @@
> >> #include <linux/delay.h>
> >> #include <linux/i2c.h>
> >> #include <linux/module.h>
> >> +#include <linux/of_device.h>
> >> #include <linux/pm_runtime.h>
> >> #include <linux/regulator/consumer.h>
> >> #include <media/v4l2-ctrls.h>
> >> @@ -321,8 +322,6 @@ static const struct imx258_reg mipi_642mbps_24mhz_4l[] = {
> >>
> >> static const struct imx258_reg mode_common_regs[] = {
> >> { 0x3051, 0x00 },
> >> - { 0x3052, 0x00 },
> >> - { 0x4E21, 0x14 },
> >> { 0x6B11, 0xCF },
> >> { 0x7FF0, 0x08 },
> >> { 0x7FF1, 0x0F },
> >> @@ -345,7 +344,6 @@ static const struct imx258_reg mode_common_regs[] = {
> >> { 0x7FA8, 0x03 },
> >> { 0x7FA9, 0xFE },
> >> { 0x7B24, 0x81 },
> >> - { 0x7B25, 0x00 },
> >> { 0x6564, 0x07 },
> >> { 0x6B0D, 0x41 },
> >> { 0x653D, 0x04 },
> >> @@ -460,6 +458,33 @@ static const struct imx258_reg mode_1048_780_regs[] = {
> >> { 0x034F, 0x0C },
> >> };
> >>
> >> +struct imx258_variant_cfg {
> >> + const struct imx258_reg *regs;
> >> + unsigned int num_regs;
> >> +};
> >> +
> >> +static const struct imx258_reg imx258_cfg_regs[] = {
> >> + { 0x3052, 0x00 },
> >> + { 0x4E21, 0x14 },
> >> + { 0x7B25, 0x00 },
> >> +};
> >> +
> >> +static const struct imx258_variant_cfg imx258_cfg = {
> >> + .regs = imx258_cfg_regs,
> >> + .num_regs = ARRAY_SIZE(imx258_cfg_regs),
> >> +};
> >> +
> >> +static const struct imx258_reg imx258_pdaf_cfg_regs[] = {
> >> + { 0x3052, 0x01 },
> >> + { 0x4E21, 0x10 },
> >> + { 0x7B25, 0x01 },
> >> +};
> >> +
> >> +static const struct imx258_variant_cfg imx258_pdaf_cfg = {
> >> + .regs = imx258_pdaf_cfg_regs,
> >> + .num_regs = ARRAY_SIZE(imx258_pdaf_cfg_regs),
> >> +};
> >> +
> >> static const char * const imx258_test_pattern_menu[] = {
> >> "Disabled",
> >> "Solid Colour",
> >> @@ -637,6 +662,8 @@ struct imx258 {
> >> struct v4l2_subdev sd;
> >> struct media_pad pad;
> >>
> >> + const struct imx258_variant_cfg *variant_cfg;
> >> +
> >> struct v4l2_ctrl_handler ctrl_handler;
> >> /* V4L2 Controls */
> >> struct v4l2_ctrl *link_freq;
> >> @@ -1104,6 +1131,14 @@ static int imx258_start_streaming(struct imx258 *imx258)
> >> return ret;
> >> }
> >>
> >> + ret = imx258_write_regs(imx258, imx258->variant_cfg->regs,
> >> + imx258->variant_cfg->num_regs);
> >> + if (ret) {
> >> + dev_err(&client->dev, "%s failed to set variant config\n",
> >> + __func__);
> >> + return ret;
> >> + }
> >> +
> >> ret = imx258_write_reg(imx258, IMX258_CLK_BLANK_STOP,
> >> IMX258_REG_VALUE_08BIT,
> >> imx258->csi2_flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK ?
> >> @@ -1492,6 +1527,10 @@ static int imx258_probe(struct i2c_client *client)
> >>
> >> imx258->csi2_flags = ep.bus.mipi_csi2.flags;
> >>
> >> + imx258->variant_cfg = of_device_get_match_data(&client->dev);
> >
> > You'll also need to keep this working for ACPI based systems. I.e. in
> > practice remove "of_" prefix here and add the non-PDAF variant data to the
> > relevant ACPI ID list.
> >
>
> Removing of_ is easy enough and looking at all the other commits that make
> this change in other drivers I dont see anything else being done besides
> adding in the .data section that is down below for both imx258 and pdaf
> versions. Is that what you are referencing or is there some other place
> to add variant data to ACPI ID list?
Speaking of which---are you absolutely certain there are two variants of
this sensor? Many sensors that have a different pixel pattern (PDAF pixels
or a non-Bayer pattern) can produce Bayer data when condigured so. The fact
that you have differing register configuration for the PDAF and non-PDAF
cases suggests this may well be the case.
>
> >> + if (!imx258->variant_cfg)
> >> + imx258->variant_cfg = &imx258_cfg;
> >> +
> >> /* Initialize subdev */
> >> v4l2_i2c_subdev_init(&imx258->sd, client, &imx258_subdev_ops);
> >>
> >> @@ -1579,7 +1618,8 @@ MODULE_DEVICE_TABLE(acpi, imx258_acpi_ids);
> >> #endif
> >>
> >> static const struct of_device_id imx258_dt_ids[] = {
> >> - { .compatible = "sony,imx258" },
> >> + { .compatible = "sony,imx258", .data = &imx258_cfg },
> >> + { .compatible = "sony,imx258-pdaf", .data = &imx258_pdaf_cfg },
> >> { /* sentinel */ }
> >> };
> >> MODULE_DEVICE_TABLE(of, imx258_dt_ids);
> >
>
--
Regards,
Sakari Ailus
^ permalink raw reply
* Re: [PATCH v2 2/2] arm64: dts: qcom: qcs6490-rb3gen2: Enable various remoteprocs
From: Komal Bajaj @ 2024-04-05 11:22 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-arm-msm, devicetree, linux-kernel, quic_tsoni
In-Reply-To: <CAA8EJpofbeyER39_tjG=sYmVp+vN2WbNZyhU6NEaePxd-QUZaQ@mail.gmail.com>
On 4/2/2024 2:47 PM, Dmitry Baryshkov wrote:
> On Tue, 2 Apr 2024 at 12:04, Komal Bajaj <quic_kbajaj@quicinc.com> wrote:
>>
>> Enable the ADSP, CDSP and WPSS that are found on qcs6490-rb3gen2.
>
> No MPSS even for GPS?
Earlier was thinking of sending a separate patch for that.
Anyway, will add MPSS too in the next series.
Thanks
Komal
>
> Anyway,
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
>>
>> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
>> ---
>> arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
>> index 97824c769ba3..a25431ddf922 100644
>> --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
>> +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
>> @@ -434,6 +434,21 @@ &qupv3_id_0 {
>> status = "okay";
>> };
>>
>> +&remoteproc_adsp {
>> + firmware-name = "qcom/qcm6490/adsp.mbn";
>> + status = "okay";
>> +};
>> +
>> +&remoteproc_cdsp {
>> + firmware-name = "qcom/qcm6490/cdsp.mbn";
>> + status = "okay";
>> +};
>> +
>> +&remoteproc_wpss {
>> + firmware-name = "qcom/qcm6490/wpss.mbn";
>> + status = "okay";
>> +};
>> +
>> &tlmm {
>> gpio-reserved-ranges = <32 2>, /* ADSP */
>> <48 4>; /* NFC */
>> --
>> 2.42.0
>>
>>
>
>
^ permalink raw reply
* Re: [PATCH v2 2/2] arm64: dts: qcom: qcs6490-rb3gen2: Enable various remoteprocs
From: Komal Bajaj @ 2024-04-05 11:22 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-arm-msm, devicetree, linux-kernel, quic_tsoni
In-Reply-To: <Zg9nmVl9eqTbkkDe@hu-bjorande-lv.qualcomm.com>
On 4/5/2024 8:23 AM, Bjorn Andersson wrote:
> On Tue, Apr 02, 2024 at 02:33:49PM +0530, Komal Bajaj wrote:
>> Enable the ADSP, CDSP and WPSS that are found on qcs6490-rb3gen2.
>>
>> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
>> ---
>> arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
>> index 97824c769ba3..a25431ddf922 100644
>> --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
>> +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
>> @@ -434,6 +434,21 @@ &qupv3_id_0 {
>> status = "okay";
>> };
>>
>> +&remoteproc_adsp {
>> + firmware-name = "qcom/qcm6490/adsp.mbn";
>
> Should this be qcm6490?
> >
> I already proposed a patch to add adsp and cdsp, using qcs6490, and this
> was merged earlier this week. Please pay attention and review patches
> posted on the public list.
Apologies, I missed that.
Will drop adsp and cdsp firmware path update in the next series.
Rebase this change on top of your change, also will add the firmware
path update for modem for GPS usecase.
Thanks
Komal
>
> Either way, this will now have to be rebased on linux-next.
>
> Thanks,
> Bjorn
>
>> + status = "okay";
>> +};
>> +
>> +&remoteproc_cdsp {
>> + firmware-name = "qcom/qcm6490/cdsp.mbn";
>> + status = "okay";
>> +};
>> +
>> +&remoteproc_wpss {
>> + firmware-name = "qcom/qcm6490/wpss.mbn";
>> + status = "okay";
>> +};
>> +
>> &tlmm {
>> gpio-reserved-ranges = <32 2>, /* ADSP */
>> <48 4>; /* NFC */
>> --
>> 2.42.0
>>
^ permalink raw reply
* Re: [PATCH RFC v2 0/4] wifi: ath10k: support board-specific firmware overrides
From: Kalle Valo @ 2024-04-05 12:01 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Jeff Johnson, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, ath10k, linux-wireless, netdev,
devicetree, linux-arm-msm, Krzysztof Kozlowski
In-Reply-To: <CAA8EJpq_XLUEMC67ck2tZRjqS0PazCkQWWMGmwydeWxTETHwcg@mail.gmail.com>
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> writes:
> On Fri, 8 Mar 2024 at 17:19, Kalle Valo <kvalo@kernel.org> wrote:
>>
>> Dmitry Baryshkov <dmitry.baryshkov@linaro.org> writes:
>>
>> >> To be on the safe side using 'qcom-rb1' makes sense but on the other
>> >> hand that means we need to update linux-firmware (basically add a new
>> >> symlink) everytime a new product is added. But are there going to be
>> >> that many new ath10k based products?
>> >>
>> >> Using 'qcm2290' is easier because for a new product then there only
>> >> needs to be a change in DTS and no need to change anything
>> >> linux-firmware. But here the risk is that if there's actually two
>> >> different ath10k firmware branches for 'qcm2290'. If that ever happens
>> >> (I hope not) I guess we could solve that by adding new 'qcm2290-foo'
>> >> directory?
>> >>
>> >> But I don't really know, thoughts?
>> >
>> > After some thought, I'd suggest to follow approach taken by the rest
>> > of qcom firmware:
>>
>> Can you provide pointers to those cases?
>
> https://gitlab.com/kernel-firmware/linux-firmware/-/tree/main/qcom/sc8280xp/LENOVO/21BX
>
>>
>> > put a default (accepted by non-secured hardware) firmware to SoC dir
>> > and then put a vendor-specific firmware into subdir. If any of such
>> > vendors appear, we might even implement structural fallback: first
>> > look into sdm845/Google/blueline, then in sdm845/Google, sdm845/ and
>> > finally just under hw1.0.
>>
>> Honestly that looks quite compilicated compared to having just one
>> sub-directory. How will ath10k find the directory names (or I vendor and
>> model names) like 'Google' or 'blueline' in this example?
>
> I was thinking about the firmware-name = "sdm845/Google/blueline". But
> this can be really simpler, firmware-name = "blueline" or
> "sdm845/blueline" with no need for fallbacks.
I have been also thinking about this and I would prefer not to have the
fallbacks. But good if you agree with that.
IMHO just "sdm845-blueline" would be the most simple. I don't see the
point of having a directory structure when there are not that many
directories really. But this is just cosmetics.
> My point is that the firmware-name provides the possibility to handle
> that in different ways.
Very good, thanks.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* [PATCH v2] dt-bindings: usb: Document the Microchip USB2514 hub
From: Fabio Estevam @ 2024-04-05 12:01 UTC (permalink / raw)
To: gregkh; +Cc: robh, krzk+dt, conor+dt, linux-usb, devicetree, Fabio Estevam
From: Fabio Estevam <festevam@denx.de>
Document the Microchip USB2412, USB2417, and USB2514 USB hubs.
The existing usb251xb.yaml describes Microchip USB251x hubs that are
connected under I2C bus. Here, the hub is under the USB bus and use
the on-board-hub interface instead.
Signed-off-by: Fabio Estevam <festevam@denx.de>
---
Changes since v1:
- Improve commit log to explain why usb251xb.yaml cannot be reused.
- Keep the compatible entries sorted.
- Pass maxItems to clocks.
- Use unevaluatedProperties: false after passing $ref: usb-hcd.yaml#.
- Drop dr_mode = "host".
- Fix example dts indentation.
- Improve the example by adding a USB Ethernet device connected
to the hub. Such example can be found at:
arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts
arch/arm/boot/dts/broadcom/bcm283x-rpi-lan7515.dtsi
.../bindings/usb/microchip,usb2514.yaml | 63 +++++++++++++++++++
1 file changed, 63 insertions(+)
create mode 100644 Documentation/devicetree/bindings/usb/microchip,usb2514.yaml
diff --git a/Documentation/devicetree/bindings/usb/microchip,usb2514.yaml b/Documentation/devicetree/bindings/usb/microchip,usb2514.yaml
new file mode 100644
index 000000000000..783c27591e56
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/microchip,usb2514.yaml
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/microchip,usb2514.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip USB2514 Hub Controller
+
+maintainers:
+ - Fabio Estevam <festevam@gmail.com>
+
+allOf:
+ - $ref: usb-hcd.yaml#
+
+properties:
+ compatible:
+ enum:
+ - usb424,2412
+ - usb424,2417
+ - usb424,2514
+
+ reg: true
+
+ reset-gpios:
+ description: GPIO connected to the RESET_N pin.
+
+ vdd-supply:
+ description: 3.3V power supply.
+
+ clocks:
+ description: External 24MHz clock connected to the CLKIN pin.
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/imx6qdl-clock.h>
+ #include <dt-bindings/gpio/gpio.h>
+
+ usb {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ usb-hub@1 {
+ compatible = "usb424,2514";
+ reg = <1>;
+ clocks = <&clks IMX6QDL_CLK_CKO>;
+ reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
+ vdd-supply = <®_3v3_hub>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ethernet@1 {
+ compatible = "usbb95,772b";
+ reg = <1>;
+ };
+ };
+ };
--
2.34.1
^ permalink raw reply related
* Re: [PATCH RFC v2 1/4] dt-bindings: net: wireless: ath10k: describe firmware-name property
From: Kalle Valo @ 2024-04-05 12:04 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Jeff Johnson, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, ath10k, linux-wireless, netdev,
devicetree, linux-arm-msm, Krzysztof Kozlowski
In-Reply-To: <20240306-wcn3990-firmware-path-v2-1-f89e98e71a57@linaro.org>
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> For WCN3990 platforms we need to look for the platform / board specific
> firmware-N.mbn file which corresponds to the wlanmdsp.mbn loaded to the
> modem DSP via the TQFTPserv. Add firmware-name property describing this
> classifier.
>
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
2 patches applied to ath-next branch of ath.git, thanks.
158fff51b4c3 dt-bindings: net: wireless: ath10k: describe firmware-name property
5abf259772df wifi: ath10k: support board-specific firmware overrides
--
https://patchwork.kernel.org/project/linux-wireless/patch/20240306-wcn3990-firmware-path-v2-1-f89e98e71a57@linaro.org/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH v3 1/9] drm: xlnx: zynqmp_dpsub: Set layer mode during creation
From: Tomi Valkeinen @ 2024-04-05 12:06 UTC (permalink / raw)
To: Anatoliy Klymenko
Cc: dri-devel, linux-arm-kernel, linux-kernel, devicetree,
linux-media, Laurent Pinchart, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter, Michal Simek,
Andrzej Hajda, Neil Armstrong, Robert Foss, Jonas Karlman,
Jernej Skrabec, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Mauro Carvalho Chehab
In-Reply-To: <20240321-dp-live-fmt-v3-1-d5090d796b7e@amd.com>
On 21/03/2024 22:43, Anatoliy Klymenko wrote:
> Set layer mode of operation (live or dma-based) during layer creation.
>
> Each DPSUB layer mode of operation is defined by corresponding DT node port
> connection, so it is possible to assign it during layer object creation.
> Previously it was set in layer enable functions, although it is too late
> as setting layer format depends on layer mode, and should be done before
> given layer enabled.
>
> Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/gpu/drm/xlnx/zynqmp_disp.c | 20 ++++++++++++++++----
> drivers/gpu/drm/xlnx/zynqmp_disp.h | 13 +------------
> drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +-
> drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +-
> 4 files changed, 19 insertions(+), 18 deletions(-)
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Tomi
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index 8a39b3accce5..e6d26ef60e89 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -64,6 +64,16 @@
>
> #define ZYNQMP_DISP_MAX_NUM_SUB_PLANES 3
>
> +/**
> + * enum zynqmp_dpsub_layer_mode - Layer mode
> + * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode
> + * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode
> + */
> +enum zynqmp_dpsub_layer_mode {
> + ZYNQMP_DPSUB_LAYER_NONLIVE,
> + ZYNQMP_DPSUB_LAYER_LIVE,
> +};
> +
> /**
> * struct zynqmp_disp_format - Display subsystem format information
> * @drm_fmt: DRM format (4CC)
> @@ -902,15 +912,12 @@ u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer,
> /**
> * zynqmp_disp_layer_enable - Enable a layer
> * @layer: The layer
> - * @mode: Operating mode of layer
> *
> * Enable the @layer in the audio/video buffer manager and the blender. DMA
> * channels are started separately by zynqmp_disp_layer_update().
> */
> -void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
> - enum zynqmp_dpsub_layer_mode mode)
> +void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer)
> {
> - layer->mode = mode;
> zynqmp_disp_avbuf_enable_video(layer->disp, layer);
> zynqmp_disp_blend_layer_enable(layer->disp, layer);
> }
> @@ -1134,6 +1141,11 @@ static int zynqmp_disp_create_layers(struct zynqmp_disp *disp)
> layer->id = i;
> layer->disp = disp;
> layer->info = &layer_info[i];
> + /* For now assume dpsub works in either live or non-live mode for both layers.
> + * Hybrid mode is not supported yet.
> + */
> + layer->mode = disp->dpsub->dma_enabled ? ZYNQMP_DPSUB_LAYER_NONLIVE
> + : ZYNQMP_DPSUB_LAYER_LIVE;
>
> ret = zynqmp_disp_layer_request_dma(disp, layer);
> if (ret)
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> index 123cffac08be..9b8b202224d9 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> @@ -42,16 +42,6 @@ enum zynqmp_dpsub_layer_id {
> ZYNQMP_DPSUB_LAYER_GFX,
> };
>
> -/**
> - * enum zynqmp_dpsub_layer_mode - Layer mode
> - * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode
> - * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode
> - */
> -enum zynqmp_dpsub_layer_mode {
> - ZYNQMP_DPSUB_LAYER_NONLIVE,
> - ZYNQMP_DPSUB_LAYER_LIVE,
> -};
> -
> void zynqmp_disp_enable(struct zynqmp_disp *disp);
> void zynqmp_disp_disable(struct zynqmp_disp *disp);
> int zynqmp_disp_setup_clock(struct zynqmp_disp *disp,
> @@ -62,8 +52,7 @@ void zynqmp_disp_blend_set_global_alpha(struct zynqmp_disp *disp,
>
> u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer,
> unsigned int *num_formats);
> -void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
> - enum zynqmp_dpsub_layer_mode mode);
> +void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer);
> void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer);
> void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
> const struct drm_format_info *info);
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 1846c4971fd8..04b6bcac3b07 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1295,7 +1295,7 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp,
> /* TODO: Make the format configurable. */
> info = drm_format_info(DRM_FORMAT_YUV422);
> zynqmp_disp_layer_set_format(layer, info);
> - zynqmp_disp_layer_enable(layer, ZYNQMP_DPSUB_LAYER_LIVE);
> + zynqmp_disp_layer_enable(layer);
>
> if (layer_id == ZYNQMP_DPSUB_LAYER_GFX)
> zynqmp_disp_blend_set_global_alpha(dp->dpsub->disp, true, 255);
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> index db3bb4afbfc4..43bf416b33d5 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> @@ -122,7 +122,7 @@ static void zynqmp_dpsub_plane_atomic_update(struct drm_plane *plane,
>
> /* Enable or re-enable the plane if the format has changed. */
> if (format_changed)
> - zynqmp_disp_layer_enable(layer, ZYNQMP_DPSUB_LAYER_NONLIVE);
> + zynqmp_disp_layer_enable(layer);
> }
>
> static const struct drm_plane_helper_funcs zynqmp_dpsub_plane_helper_funcs = {
>
^ permalink raw reply
* [PATCH 0/3] arm64: dts: qcom: msm8916-samsung-fortuna: Add accelerometer/magnetometer
From: Raymond Hackley @ 2024-04-05 12:08 UTC (permalink / raw)
To: linux-kernel
Cc: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Stephan Gerhold, Nikita Travkin, linux-arm-msm,
devicetree, ~postmarketos/upstreaming
Some Grand Prime use a Bosch BMC150 accelerometer/magnetometer combo.
The chip provides two separate I2C devices for the accelerometer
and magnetometer that are already supported by the bmc150-accel
and bmc150-magn driver.
Some Grand Prime use a ST LSM303C accelerometer/magnetometer combo.
Core Prime LTE uses ST LIS2HH12 accelerometer.
Add support for them.
^ permalink raw reply
* [PATCH 1/3] arm64: dts: qcom: msm8916-samsung-fortuna: Add BMC150 accelerometer/magnetometer
From: Raymond Hackley @ 2024-04-05 12:08 UTC (permalink / raw)
To: linux-kernel
Cc: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Stephan Gerhold, Nikita Travkin, linux-arm-msm,
devicetree, ~postmarketos/upstreaming, Joe Mason, Raymond Hackley
In-Reply-To: <20240405120803.20754-1-raymondhackley@protonmail.com>
From: Joe Mason <buddyjojo06@outlook.com>
Some Grand Prime use a Bosch BMC150 accelerometer/magnetometer combo.
The chip provides two separate I2C devices for the accelerometer
and magnetometer that are already supported by the bmc150-accel
and bmc150-magn driver.
Signed-off-by: Joe Mason <buddyjojo06@outlook.com>
[Stephan: Move sensors to common dtsi, disabled by default]
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
[Raymond: Add it to grandprimelte. Use interrupts-extended]
Signed-off-by: Raymond Hackley <raymondhackley@protonmail.com>
---
.../qcom/msm8916-samsung-fortuna-common.dtsi | 44 +++++++++++++++++++
.../dts/qcom/msm8916-samsung-gprimeltecan.dts | 8 ++++
.../qcom/msm8916-samsung-grandprimelte.dts | 8 ++++
3 files changed, 60 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/msm8916-samsung-fortuna-common.dtsi b/arch/arm64/boot/dts/qcom/msm8916-samsung-fortuna-common.dtsi
index 5e933fb8b363..b6e1fe8b0056 100644
--- a/arch/arm64/boot/dts/qcom/msm8916-samsung-fortuna-common.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916-samsung-fortuna-common.dtsi
@@ -117,6 +117,43 @@ muic: extcon@25 {
};
};
+&blsp_i2c2 {
+ /* Available sensors vary depending on model variant */
+ status = "okay";
+
+ bosch_accel: accelerometer@10 {
+ compatible = "bosch,bmc150_accel";
+ reg = <0x10>;
+ interrupts-extended = <&tlmm 115 IRQ_TYPE_EDGE_RISING>;
+
+ vdd-supply = <&pm8916_l5>;
+ vddio-supply = <&pm8916_l5>;
+
+ pinctrl-0 = <&accel_int_default>;
+ pinctrl-names = "default";
+
+ mount-matrix = "0", "-1", "0",
+ "-1", "0", "0",
+ "0", "0", "1";
+
+ status = "disabled";
+ };
+
+ bosch_magn: magnetometer@12 {
+ compatible = "bosch,bmc150_magn";
+ reg = <0x12>;
+
+ vdd-supply = <&pm8916_l5>;
+ vddio-supply = <&pm8916_l5>;
+
+ mount-matrix = "0", "-1", "0",
+ "-1", "0", "0",
+ "0", "0", "1";
+
+ status = "disabled";
+ };
+};
+
&blsp_i2c4 {
status = "okay";
@@ -223,6 +260,13 @@ &wcnss_mem {
};
&tlmm {
+ accel_int_default: accel-int-default-state {
+ pins = "gpio115";
+ function = "gpio";
+ drive-strength = <2>;
+ bias-disable;
+ };
+
backlight_en_default: backlight-en-default-state {
pins = "gpio98";
function = "gpio";
diff --git a/arch/arm64/boot/dts/qcom/msm8916-samsung-gprimeltecan.dts b/arch/arm64/boot/dts/qcom/msm8916-samsung-gprimeltecan.dts
index 9d65fa58ba92..4dc74e8bf1d8 100644
--- a/arch/arm64/boot/dts/qcom/msm8916-samsung-gprimeltecan.dts
+++ b/arch/arm64/boot/dts/qcom/msm8916-samsung-gprimeltecan.dts
@@ -21,6 +21,14 @@ tz-apps@85500000 {
};
};
+&bosch_accel {
+ status = "okay";
+};
+
+&bosch_magn {
+ status = "okay";
+};
+
&mpss_mem {
/* Firmware for gprimeltecan needs more space */
reg = <0x0 0x86800000 0x0 0x5400000>;
diff --git a/arch/arm64/boot/dts/qcom/msm8916-samsung-grandprimelte.dts b/arch/arm64/boot/dts/qcom/msm8916-samsung-grandprimelte.dts
index a66ce4b13547..cffad734c4df 100644
--- a/arch/arm64/boot/dts/qcom/msm8916-samsung-grandprimelte.dts
+++ b/arch/arm64/boot/dts/qcom/msm8916-samsung-grandprimelte.dts
@@ -10,6 +10,14 @@ / {
chassis-type = "handset";
};
+&bosch_accel {
+ status = "okay";
+};
+
+&bosch_magn {
+ status = "okay";
+};
+
&mpss_mem {
/* Firmware for grandprimelte needs more space */
reg = <0x0 0x86800000 0x0 0x5400000>;
--
2.39.2
^ permalink raw reply related
* [PATCH 2/3] arm64: dts: qcom: msm8916-samsung-fortuna: Add LSM303C accelerometer/magnetometer
From: Raymond Hackley @ 2024-04-05 12:08 UTC (permalink / raw)
To: linux-kernel
Cc: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Stephan Gerhold, Nikita Travkin, linux-arm-msm,
devicetree, ~postmarketos/upstreaming, Siddharth Manthan,
Raymond Hackley
In-Reply-To: <20240405120803.20754-1-raymondhackley@protonmail.com>
From: Siddharth Manthan <siddharth.manthan@gmail.com>
Some Grand Prime use a ST LSM303C accelerometer/magnetometer combo.
Add support for it.
Signed-off-by: Siddharth Manthan <siddharth.manthan@gmail.com>
[Stephan: Move sensors to common dtsi (disabled by default)]
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
[Raymond: Use interrupts-extended]
Signed-off-by: Raymond Hackley <raymondhackley@protonmail.com>
---
.../dts/qcom/msm8216-samsung-fortuna3g.dts | 8 +++++
.../qcom/msm8916-samsung-fortuna-common.dtsi | 34 +++++++++++++++++++
2 files changed, 42 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/msm8216-samsung-fortuna3g.dts b/arch/arm64/boot/dts/qcom/msm8216-samsung-fortuna3g.dts
index 366914be7d53..e7f6df229f9a 100644
--- a/arch/arm64/boot/dts/qcom/msm8216-samsung-fortuna3g.dts
+++ b/arch/arm64/boot/dts/qcom/msm8216-samsung-fortuna3g.dts
@@ -9,3 +9,11 @@ / {
compatible = "samsung,fortuna3g", "qcom,msm8916";
chassis-type = "handset";
};
+
+&st_accel {
+ status = "okay";
+};
+
+&st_magn {
+ status = "okay";
+};
diff --git a/arch/arm64/boot/dts/qcom/msm8916-samsung-fortuna-common.dtsi b/arch/arm64/boot/dts/qcom/msm8916-samsung-fortuna-common.dtsi
index b6e1fe8b0056..1c3e3b860d44 100644
--- a/arch/arm64/boot/dts/qcom/msm8916-samsung-fortuna-common.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916-samsung-fortuna-common.dtsi
@@ -152,6 +152,40 @@ bosch_magn: magnetometer@12 {
status = "disabled";
};
+
+ st_accel: accelerometer@1d {
+ compatible = "st,lsm303c-accel";
+ reg = <0x1d>;
+ interrupts-extended = <&tlmm 115 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "INT1";
+
+ vdd-supply = <&pm8916_l17>;
+ vddio-supply = <&pm8916_l5>;
+
+ pinctrl-0 = <&accel_int_default>;
+ pinctrl-names = "default";
+
+ st,drdy-int-pin = <1>;
+ mount-matrix = "0", "-1", "0",
+ "1", "0", "0",
+ "0", "0", "-1";
+
+ status = "disabled";
+ };
+
+ st_magn: magnetometer@1e {
+ compatible = "st,lsm303c-magn";
+ reg = <0x1e>;
+
+ vdd-supply = <&pm8916_l17>;
+ vddio-supply = <&pm8916_l5>;
+
+ mount-matrix = "0", "-1", "0",
+ "1", "0", "0",
+ "0", "0", "-1";
+
+ status = "disabled";
+ };
};
&blsp_i2c4 {
--
2.39.2
^ permalink raw reply related
* [PATCH 3/3] arm64: dts: qcom: msm8916-samsung-rossa: Add LIS2HH12 accelerometer
From: Raymond Hackley @ 2024-04-05 12:09 UTC (permalink / raw)
To: linux-kernel
Cc: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Stephan Gerhold, Nikita Travkin, linux-arm-msm,
devicetree, ~postmarketos/upstreaming, Raymond Hackley
In-Reply-To: <20240405120803.20754-1-raymondhackley@protonmail.com>
Core Prime LTE uses ST LIS2HH12 accelerometer. Add support for it.
[Stephen: Use common &st_accel definition from common dtsi]
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
Signed-off-by: Raymond Hackley <raymondhackley@protonmail.com>
---
.../arm64/boot/dts/qcom/msm8916-samsung-rossa-common.dtsi | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/msm8916-samsung-rossa-common.dtsi b/arch/arm64/boot/dts/qcom/msm8916-samsung-rossa-common.dtsi
index b438fa81886c..db95bdbb9f32 100644
--- a/arch/arm64/boot/dts/qcom/msm8916-samsung-rossa-common.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916-samsung-rossa-common.dtsi
@@ -26,3 +26,11 @@ &clk_pwm {
&clk_pwm_backlight {
status = "disabled";
};
+
+&st_accel {
+ compatible = "st,lis2hh12";
+ mount-matrix = "1", "0", "0",
+ "0", "-1", "0",
+ "0", "0", "1";
+ status = "okay";
+};
--
2.39.2
^ permalink raw reply related
* Re: [PATCH v3 2/9] drm: xlnx: zynqmp_dpsub: Update live format defines
From: Tomi Valkeinen @ 2024-04-05 12:10 UTC (permalink / raw)
To: Anatoliy Klymenko
Cc: dri-devel, linux-arm-kernel, linux-kernel, devicetree,
linux-media, Laurent Pinchart, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter, Michal Simek,
Andrzej Hajda, Neil Armstrong, Robert Foss, Jonas Karlman,
Jernej Skrabec, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Mauro Carvalho Chehab
In-Reply-To: <20240321-dp-live-fmt-v3-2-d5090d796b7e@amd.com>
On 21/03/2024 22:43, Anatoliy Klymenko wrote:
> Update live format defines to match DPSUB AV_BUF_LIVE_VID_CONFIG register
> layout.
I think this description needs a bit more. Mention that the defines are
not currently used, so we can change them like this without any other
change.
Tomi
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com>
> ---
> drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> index f92a006d5070..fa3935384834 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> @@ -165,10 +165,10 @@
> #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_10 0x2
> #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_12 0x3
> #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_MASK GENMASK(2, 0)
> -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB 0x0
> -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444 0x1
> -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422 0x2
> -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY 0x3
> +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB (0x0 << 4)
> +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444 (0x1 << 4)
> +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422 (0x2 << 4)
> +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY (0x3 << 4)
> #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_MASK GENMASK(5, 4)
> #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_CB_FIRST BIT(8)
> #define ZYNQMP_DISP_AV_BUF_PALETTE_MEMORY 0x400
>
^ permalink raw reply
* Re: [PATCH RFT 09/10] arm64: dts: microchip: sparx5_pcb134: drop duplicated NOR flash
From: Steen Hegelund @ 2024-04-05 12:10 UTC (permalink / raw)
To: Krzysztof Kozlowski, Conor Dooley, Nicolas Ferre, Claudiu Beznea,
Rob Herring, Krzysztof Kozlowski, Lars Povlsen, Daniel Machon,
UNGLinuxDriver, David S. Miller, Bjarni Jonasson,
linux-arm-kernel, devicetree, linux-kernel
In-Reply-To: <20240401153740.123978-9-krzk@kernel.org>
Hi Krzysztof,
On Mon, 2024-04-01 at 17:37 +0200, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Since beginning the DTS extended the SPI0 in two places adding two SPI
> muxes, each with same SPI NOR flash. Both used exactly the same
> chip-selects, so this was clearly buggy code. Without checking in
> datasheet, assume device has only one SPI NOR flash, so code was
> duplicated.
>
> Fixes dtc W=1 warnings:
>
> sparx5_pcb134_board.dtsi:277.10-281.4: Warning (unique_unit_address_if_enabled):
> /axi@600000000/spi@600104000/flash@0: duplicate unit-address (also used in node
> /axi@600000000/spi@600104000/spi@0)
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>
> ---
>
> Not tested on hardware
> ---
> .../boot/dts/microchip/sparx5_pcb134_board.dtsi | 16 ----------------
> 1 file changed, 16 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi
> b/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi
> index f165a409bc1d..dc7b59dfcb40 100644
> --- a/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi
> +++ b/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi
> @@ -281,22 +281,6 @@ flash@0 {
> };
> };
>
> -&spi0 {
> - status = "okay";
> - spi@0 {
> - compatible = "spi-mux";
> - mux-controls = <&mux>;
> - #address-cells = <1>;
> - #size-cells = <0>;
> - reg = <0>; /* CS0 */
> - flash@9 {
> - compatible = "jedec,spi-nor";
> - spi-max-frequency = <8000000>;
> - reg = <0x9>; /* SPI */
> - };
> - };
> -};
> -
When testing this on actual HW the SPI NOR is no longer accessible.
The reason is that it sits behind a SPI-MUX and that needs to be present in the Device Tree.
So if you do the "reverse" clean-up it works fine: Remove the simple spi0 node and keep the one that
has the spi-mux reference.
> &sgpio0 {
> status = "okay";
> microchip,sgpio-port-ranges = <8 15>;
> --
> 2.34.1
>
Thanks for the cleanup of the DT files!
Best Regards
Steen
^ permalink raw reply
* Re: [PATCH RFT 10/10] arm64: dts: microchip: sparx5_pcb135: drop duplicated NOR flash
From: Steen Hegelund @ 2024-04-05 12:11 UTC (permalink / raw)
To: Krzysztof Kozlowski, Conor Dooley, Nicolas Ferre, Claudiu Beznea,
Rob Herring, Krzysztof Kozlowski, Lars Povlsen, Daniel Machon,
UNGLinuxDriver, David S. Miller, Bjarni Jonasson,
linux-arm-kernel, devicetree, linux-kernel
In-Reply-To: <20240401153740.123978-10-krzk@kernel.org>
Hi Krzysztof,
On Mon, 2024-04-01 at 17:37 +0200, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Since beginning the DTS extended the SPI0 in two places adding two SPI
> muxes, each with same SPI NOR flash. Both used exactly the same
> chip-selects, so this was clearly buggy code. Without checking in
> datasheet, assume device has only one SPI NOR flash, so code was
> duplicated.
>
> Fixes dtc W=1 warnings:
>
> sparx5_pcb135_board.dtsi:92.10-96.4: Warning (unique_unit_address_if_enabled):
> /axi@600000000/spi@600104000/flash@0: duplicate unit-address (also used in node
> /axi@600000000/spi@600104000/spi@0)
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>
> ---
>
> Not tested on hardware
> ---
> .../boot/dts/microchip/sparx5_pcb135_board.dtsi | 16 ----------------
> 1 file changed, 16 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi
> b/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi
> index 20016efb3656..d64e642e3873 100644
> --- a/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi
> +++ b/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi
> @@ -96,22 +96,6 @@ flash@0 {
> };
> };
>
> -&spi0 {
> - status = "okay";
> - spi@0 {
> - compatible = "spi-mux";
> - mux-controls = <&mux>;
> - #address-cells = <1>;
> - #size-cells = <0>;
> - reg = <0>; /* CS0 */
> - flash@9 {
> - compatible = "jedec,spi-nor";
> - spi-max-frequency = <8000000>;
> - reg = <0x9>; /* SPI */
> - };
> - };
> -};
> -
I also tested this, and no surprise: same comment as for the pcb134 patch...
> &sgpio1 {
> status = "okay";
> microchip,sgpio-port-ranges = <24 31>;
> --
> 2.34.1
>
Best Regards
Steen
^ permalink raw reply
* Re: [PATCH v3 3/9] drm: xlnx: zynqmp_dpsub: Add connected live layer helper
From: Tomi Valkeinen @ 2024-04-05 12:12 UTC (permalink / raw)
To: Anatoliy Klymenko
Cc: dri-devel, linux-arm-kernel, linux-kernel, devicetree,
linux-media, Laurent Pinchart, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter, Michal Simek,
Andrzej Hajda, Neil Armstrong, Robert Foss, Jonas Karlman,
Jernej Skrabec, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Mauro Carvalho Chehab
In-Reply-To: <20240321-dp-live-fmt-v3-3-d5090d796b7e@amd.com>
On 21/03/2024 22:43, Anatoliy Klymenko wrote:
> Add a helper function capturing the first connected live display layer
> discovery logic.
>
> Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com>
> ---
> drivers/gpu/drm/xlnx/zynqmp_dp.c | 37 +++++++++++++++++++++++--------------
> 1 file changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 04b6bcac3b07..4faafdd76798 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1276,28 +1276,40 @@ static void zynqmp_dp_encoder_mode_set_stream(struct zynqmp_dp *dp,
> * DISP Configuration
> */
>
> +/**
> + * zynqmp_dp_disp_connected_live_layer - Return the first connected live layer
> + * @dp: DisplayPort IP core structure
> + *
> + * Return: The first connected live display layer or NULL if none of the live
> + * layer is connected.
"layers"
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Tomi
> + */
> +static struct zynqmp_disp_layer *
> +zynqmp_dp_disp_connected_live_layer(struct zynqmp_dp *dp)
> +{
> + if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_VIDEO))
> + return dp->dpsub->layers[ZYNQMP_DPSUB_LAYER_VID];
> + else if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_GFX))
> + return dp->dpsub->layers[ZYNQMP_DPSUB_LAYER_GFX];
> + else
> + return NULL;
> +}
> +
> static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp,
> struct drm_bridge_state *old_bridge_state)
> {
> - enum zynqmp_dpsub_layer_id layer_id;
> struct zynqmp_disp_layer *layer;
> const struct drm_format_info *info;
>
> - if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_VIDEO))
> - layer_id = ZYNQMP_DPSUB_LAYER_VID;
> - else if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_GFX))
> - layer_id = ZYNQMP_DPSUB_LAYER_GFX;
> - else
> + layer = zynqmp_dp_disp_connected_live_layer(dp);
> + if (!layer)
> return;
>
> - layer = dp->dpsub->layers[layer_id];
> -
> /* TODO: Make the format configurable. */
> info = drm_format_info(DRM_FORMAT_YUV422);
> zynqmp_disp_layer_set_format(layer, info);
> zynqmp_disp_layer_enable(layer);
>
> - if (layer_id == ZYNQMP_DPSUB_LAYER_GFX)
> + if (layer == dp->dpsub->layers[ZYNQMP_DPSUB_LAYER_GFX])
> zynqmp_disp_blend_set_global_alpha(dp->dpsub->disp, true, 255);
> else
> zynqmp_disp_blend_set_global_alpha(dp->dpsub->disp, false, 0);
> @@ -1310,11 +1322,8 @@ static void zynqmp_dp_disp_disable(struct zynqmp_dp *dp,
> {
> struct zynqmp_disp_layer *layer;
>
> - if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_VIDEO))
> - layer = dp->dpsub->layers[ZYNQMP_DPSUB_LAYER_VID];
> - else if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_GFX))
> - layer = dp->dpsub->layers[ZYNQMP_DPSUB_LAYER_GFX];
> - else
> + layer = zynqmp_dp_disp_connected_live_layer(dp);
> + if (!layer)
> return;
>
> zynqmp_disp_disable(dp->dpsub->disp);
>
^ permalink raw reply
* Re: [PATCH v2 02/18] PCI: endpoint: Introduce pci_epc_map_align()
From: Niklas Cassel @ 2024-04-05 12:20 UTC (permalink / raw)
To: Damien Le Moal
Cc: Kishon Vijay Abraham I, Manivannan Sadhasivam, Lorenzo Pieralisi,
Kishon Vijay Abraham I, Shawn Lin, Krzysztof Wilczyński,
Bjorn Helgaas, Heiko Stuebner, linux-pci, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, devicetree, linux-rockchip,
linux-arm-kernel, Rick Wertenbroek, Wilfred Mallawa
In-Reply-To: <3a2aff21-4b1d-4f99-bd49-bf75f41cb924@kernel.org>
On Thu, Apr 04, 2024 at 11:43:47AM +0900, Damien Le Moal wrote:
> On 4/3/24 21:33, Kishon Vijay Abraham I wrote:
> > Hi Damien,
> >
> > On 3/30/2024 9:49 AM, Damien Le Moal wrote:
> >> Some endpoint controllers have requirements on the alignment of the
> >> controller physical memory address that must be used to map a RC PCI
> >> address region. For instance, the rockchip endpoint controller uses
> >> at most the lower 20 bits of a physical memory address region as the
> >> lower bits of an RC PCI address. For mapping a PCI address region of
> >> size bytes starting from pci_addr, the exact number of address bits
> >> used is the number of address bits changing in the address range
> >> [pci_addr..pci_addr + size - 1].
> >>
> >> For this example, this creates the following constraints:
> >> 1) The offset into the controller physical memory allocated for a
> >> mapping depends on the mapping size *and* the starting PCI address
> >> for the mapping.
> >> 2) A mapping size cannot exceed the controller windows size (1MB) minus
> >> the offset needed into the allocated physical memory, which can end
> >> up being a smaller size than the desired mapping size.
> >>
> >> Handling these constraints independently of the controller being used in
> >> a PCI EP function driver is not possible with the current EPC API as
> >> it only provides the ->align field in struct pci_epc_features.
> >> Furthermore, this alignment is static and does not depend on a mapping
> >> pci address and size.
> >>
> >> Solve this by introducing the function pci_epc_map_align() and the
> >> endpoint controller operation ->map_align to allow endpoint function
> >> drivers to obtain the size and the offset into a controller address
> >> region that must be used to map an RC PCI address region. The size
> >> of the physical address region provided by pci_epc_map_align() can then
> >> be used as the size argument for the function pci_epc_mem_alloc_addr().
> >> The offset into the allocated controller memory can be used to
> >> correctly handle data transfers. Of note is that pci_epc_map_align() may
> >> indicate upon return a mapping size that is smaller (but not 0) than the
> >> requested PCI address region size. For such case, an endpoint function
> >> driver must handle data transfers in fragments.
> >>
> >> The controller operation ->map_align is optional: controllers that do
> >> not have any address alignment constraints for mapping a RC PCI address
> >> region do not need to implement this operation. For such controllers,
> >> pci_epc_map_align() always returns the mapping size as equal
> >> to the requested size and an offset equal to 0.
> >>
> >> The structure pci_epc_map is introduced to represent a mapping start PCI
> >> address, size and the size and offset into the controller memory needed
> >> for mapping the PCI address region.
> >>
> >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> >> ---
> >> drivers/pci/endpoint/pci-epc-core.c | 66 +++++++++++++++++++++++++++++
> >> include/linux/pci-epc.h | 33 +++++++++++++++
> >> 2 files changed, 99 insertions(+)
> >>
> >> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> >> index 754afd115bbd..37758ca91d7f 100644
> >> --- a/drivers/pci/endpoint/pci-epc-core.c
> >> +++ b/drivers/pci/endpoint/pci-epc-core.c
> >> @@ -433,6 +433,72 @@ void pci_epc_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> >> }
> >> EXPORT_SYMBOL_GPL(pci_epc_unmap_addr);
> >>
> >> +/**
> >> + * pci_epc_map_align() - Get the offset into and the size of a controller memory
> >> + * address region needed to map a RC PCI address region
> >> + * @epc: the EPC device on which address is allocated
> >> + * @func_no: the physical endpoint function number in the EPC device
> >> + * @vfunc_no: the virtual endpoint function number in the physical function
> >> + * @pci_addr: PCI address to which the physical address should be mapped
> >> + * @size: the size of the mapping starting from @pci_addr
> >> + * @map: populate here the actual size and offset into the controller memory
> >> + * that must be allocated for the mapping
> >> + *
> >> + * Invoke the controller map_align operation to obtain the size and the offset
> >> + * into a controller address region that must be allocated to map @size
> >> + * bytes of the RC PCI address space starting from @pci_addr.
> >> + *
> >> + * The size of the mapping that can be handled by the controller is indicated
> >> + * using the pci_size field of @map. This size may be smaller than the requested
> >> + * @size. In such case, the function driver must handle the mapping using
> >> + * several fragments. The offset into the controller memory for the effective
> >> + * mapping of the @pci_addr..@pci_addr+@map->pci_size address range is indicated
> >> + * using the map_ofst field of @map.
> >> + */
> >> +int pci_epc_map_align(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> >> + u64 pci_addr, size_t size, struct pci_epc_map *map)
> >> +{
> >> + const struct pci_epc_features *features;
> >> + size_t mask;
> >> + int ret;
> >> +
> >> + if (!pci_epc_function_is_valid(epc, func_no, vfunc_no))
> >> + return -EINVAL;
> >> +
> >> + if (!size || !map)
> >> + return -EINVAL;
> >> +
> >> + memset(map, 0, sizeof(*map));
> >> + map->pci_addr = pci_addr;
> >> + map->pci_size = size;
> >> +
> >> + if (epc->ops->map_align) {
> >> + mutex_lock(&epc->lock);
> >> + ret = epc->ops->map_align(epc, func_no, vfunc_no, map);
> >> + mutex_unlock(&epc->lock);
> >> + return ret;
> >> + }
> >> +
> >> + /*
> >> + * Assume a fixed alignment constraint as specified by the controller
> >> + * features.
> >> + */
> >> + features = pci_epc_get_features(epc, func_no, vfunc_no);
> >> + if (!features || !features->align) {
> >> + map->map_pci_addr = pci_addr;
> >> + map->map_size = size;
> >> + map->map_ofst = 0;
> >> + }
> >
> > The 'align' of pci_epc_features was initially added only to address the
> > inbound ATU constraints. This is also added as comment in [1]. The PCI
> > address restrictions (only fixed alignment constraint) were handled by
> > the host side driver and depends on the connected endpoint device
> > (atleast it was like that for pci_endpoint_test.c [2]).
> > So pci-epf-test.c used the 'align' in pci_epc_features only as part of
> > pci_epf_alloc_space().
> >
> > Though I have abused 'align' of pci_epc_features in pci-epf-ntb.c using
> > it out of pci_epf_alloc_space(), I think we should keep the 'align' of
> > pci_epc_features only within pci_epf_alloc_space() and controllers with
> > any PCI address restrictions to implement ->map_align(). This could as
> > well be done in a phased manner to let controllers implement
> > ->map_align() and then remove using pci_epc_features in
> > pci_epc_map_align(). Let me know what you think?
First you say that you want to avoid using epc_features->align inside
pci_epc_map_align(), and then you say that we could do it in phases,
and eventually stop using epc_features->align in pci_epc_map_align().
I'm confused... :)
Do you really want pci_epc_map_align() to make use of epc_features->align ?
Don't you mean ep->page_size ?
(Please read the whole email to see my reasoning.)
>
> Yep, good idea. I will remove the use of "align" as a default alignment
> constraint. For controllers that have a fixed alignment constraint (not
> necessarilly epc->features->align), it is trivial to provide a generic helper
> function that implements the ->map_align method.
We can see that commit:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2a9a801620efac92885fc9cd53594c0b9aba87a4
Introduced epc_features->align and modified pci_epf_alloc_space() to use it.
From reading the commit, it appears that epc_features->align was intended to
represent inbound iATU alignment requirement.
For DWC based controllers, the inbound iATU address must be aligned to:
CX_ATU_MIN_REGION_SIZE.
AFAICT, epc_features->align currently has nothing to do with traffic outbound
from the EP.
For aligning the reads/writes to buffers allocated on the host side,
we currently have .alignment in the host side driver:
https://github.com/torvalds/linux/blob/v6.9-rc2/drivers/misc/pci_endpoint_test.c#L966-L1021
Which should be set to the outbound iATU alignment requirement.
For DWC based controllers, the outbound iATU address must be aligned to:
CX_ATU_MIN_REGION_SIZE.
Additionally, we have ep->page_size, which defines the smallest outbound unit
that can be mapped.
(On DWC based controllers, tis is CX_ATU_MIN_REGION_SIZE.)
ep->page_size is used to specify the outbound alignment for e.g.
dw_pcie_ep_raise_msi_irq() and dw_pcie_ep_raise_msix_irq():
https://github.com/torvalds/linux/blob/v6.9-rc2/drivers/pci/controller/dwc/pcie-designware-ep.c#L488
https://github.com/torvalds/linux/blob/v6.9-rc2/drivers/pci/controller/dwc/pcie-designware-ep.c#L555
which makes sure that we can write to the RC side MSI/MSI-X address
while satisfying the outbound iATU alignment requirement.
See also:
https://lore.kernel.org/linux-pci/20240402-pci2_upstream-v3-2-803414bdb430@nxp.com/
Now I understand that rockchip is the first one that does not have a fixed
alignment.
So for that platform, map_align() will be different from ep->page_size.
(For all DWC based drivers the outbound iATU alignment requirement is
the same as the page size.)
However, it would be nice if:
1) We could have a default implementation of map_align() that by default uses
ep->page_size. Platforms that have non-fixed alignment requirements could
define their own map_align().
2) We fix dw_pcie_ep_raise_msi_irq() and dw_pcie_ep_raise_msix_irq() to use
the new pci_epc_map_align().
3) It is getting too complicated with all these...
epc_features->align, ep->page_size, map_align(), and .alignment in host driver.
I think that we need to document each of these in Documentation/PCI/endpoint/
4) It would be nice if we could set page_size correctly for all the PCI device
and vendor IDs that have defined an .alignment in drivers/misc/pci_endpoint_test.c
in the correct EPC driver. That way, we should be able to completely remove all
.alignment specified in drivers/misc/pci_endpoint_test.c.
5) Unfortunately drivers/misc/pci_endpoint_test.c defines a default alignment
of 4K:
https://github.com/torvalds/linux/blob/v6.9-rc2/drivers/misc/pci_endpoint_test.c#L968
https://github.com/torvalds/linux/blob/v6.9-rc2/drivers/misc/pci_endpoint_test.c#L820
It would be nice if we could get rid of this as well. Or perhaps add an option
to pci_test so that it does not use this 4k alignment, such that we can verify
that pci_epc_map_align() is actually working.
In my opinion 4) is the biggest win with this series, because it means that
we define the alignment in the EPC driver, instead of needing to define it in
each and every host side driver. But right now, this great improvement is not
really visible for someone looking quickly at the current series.
Kind regards,
Niklas
^ permalink raw reply
* Re: [PATCH 1/4] arm64: dts: stratix10: socdk: drop unneeded flash address/size-cells
From: Dinh Nguyen @ 2024-04-05 12:30 UTC (permalink / raw)
To: Krzysztof Kozlowski, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, devicetree, linux-kernel
In-Reply-To: <20240401141025.98125-1-krzk@kernel.org>
On 4/1/24 09:10, Krzysztof Kozlowski wrote:
> Flash node uses single "partition" node to describe partitions, so
> remove deprecated address/size-cells properties to also fix dtc W=1
> warnings:
>
> socfpga_stratix10_socdk.dts:182.10-211.4: Warning (avoid_unnecessary_addr_size): /soc@0/spi@ff8d2000/flash@0: unnecessary #address-cells/#size-cells without "ranges" or child "reg" property
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
> arch/arm64/boot/dts/altera/socfpga_stratix10_socdk.dts | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk.dts b/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk.dts
> index 26173f0b0051..4eee777ef1a1 100644
> --- a/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk.dts
> +++ b/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk.dts
> @@ -180,8 +180,6 @@ rtc@68 {
> &qspi {
> status = "okay";
> flash@0 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> compatible = "micron,mt25qu02g", "jedec,spi-nor";
> reg = <0>;
> spi-max-frequency = <100000000>;
All patches applied!
Thanks,
Dinh
^ permalink raw reply
* Re: [PATCH v3 1/9] drm: xlnx: zynqmp_dpsub: Set layer mode during creation
From: Tomi Valkeinen @ 2024-04-05 12:31 UTC (permalink / raw)
To: Anatoliy Klymenko
Cc: dri-devel, linux-arm-kernel, linux-kernel, devicetree,
linux-media, Laurent Pinchart, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter, Michal Simek,
Andrzej Hajda, Neil Armstrong, Robert Foss, Jonas Karlman,
Jernej Skrabec, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Mauro Carvalho Chehab
In-Reply-To: <20240321-dp-live-fmt-v3-1-d5090d796b7e@amd.com>
On 21/03/2024 22:43, Anatoliy Klymenko wrote:
> Set layer mode of operation (live or dma-based) during layer creation.
>
> Each DPSUB layer mode of operation is defined by corresponding DT node port
> connection, so it is possible to assign it during layer object creation.
> Previously it was set in layer enable functions, although it is too late
> as setting layer format depends on layer mode, and should be done before
> given layer enabled.
>
> Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/gpu/drm/xlnx/zynqmp_disp.c | 20 ++++++++++++++++----
> drivers/gpu/drm/xlnx/zynqmp_disp.h | 13 +------------
> drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +-
> drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +-
> 4 files changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index 8a39b3accce5..e6d26ef60e89 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -64,6 +64,16 @@
>
> #define ZYNQMP_DISP_MAX_NUM_SUB_PLANES 3
>
> +/**
> + * enum zynqmp_dpsub_layer_mode - Layer mode
> + * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode
> + * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode
> + */
> +enum zynqmp_dpsub_layer_mode {
> + ZYNQMP_DPSUB_LAYER_NONLIVE,
> + ZYNQMP_DPSUB_LAYER_LIVE,
> +};
> +
> /**
> * struct zynqmp_disp_format - Display subsystem format information
> * @drm_fmt: DRM format (4CC)
> @@ -902,15 +912,12 @@ u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer,
> /**
> * zynqmp_disp_layer_enable - Enable a layer
> * @layer: The layer
> - * @mode: Operating mode of layer
> *
> * Enable the @layer in the audio/video buffer manager and the blender. DMA
> * channels are started separately by zynqmp_disp_layer_update().
> */
> -void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
> - enum zynqmp_dpsub_layer_mode mode)
> +void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer)
> {
> - layer->mode = mode;
> zynqmp_disp_avbuf_enable_video(layer->disp, layer);
> zynqmp_disp_blend_layer_enable(layer->disp, layer);
> }
> @@ -1134,6 +1141,11 @@ static int zynqmp_disp_create_layers(struct zynqmp_disp *disp)
> layer->id = i;
> layer->disp = disp;
> layer->info = &layer_info[i];
> + /* For now assume dpsub works in either live or non-live mode for both layers.
> + * Hybrid mode is not supported yet.
> + */
This comment style is not according to the style guide, and in fact you
fix it in the patch 4. So please fix it here instead.
Tomi
> + layer->mode = disp->dpsub->dma_enabled ? ZYNQMP_DPSUB_LAYER_NONLIVE
> + : ZYNQMP_DPSUB_LAYER_LIVE;
>
> ret = zynqmp_disp_layer_request_dma(disp, layer);
> if (ret)
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> index 123cffac08be..9b8b202224d9 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> @@ -42,16 +42,6 @@ enum zynqmp_dpsub_layer_id {
> ZYNQMP_DPSUB_LAYER_GFX,
> };
>
> -/**
> - * enum zynqmp_dpsub_layer_mode - Layer mode
> - * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode
> - * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode
> - */
> -enum zynqmp_dpsub_layer_mode {
> - ZYNQMP_DPSUB_LAYER_NONLIVE,
> - ZYNQMP_DPSUB_LAYER_LIVE,
> -};
> -
> void zynqmp_disp_enable(struct zynqmp_disp *disp);
> void zynqmp_disp_disable(struct zynqmp_disp *disp);
> int zynqmp_disp_setup_clock(struct zynqmp_disp *disp,
> @@ -62,8 +52,7 @@ void zynqmp_disp_blend_set_global_alpha(struct zynqmp_disp *disp,
>
> u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer,
> unsigned int *num_formats);
> -void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
> - enum zynqmp_dpsub_layer_mode mode);
> +void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer);
> void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer);
> void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
> const struct drm_format_info *info);
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 1846c4971fd8..04b6bcac3b07 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1295,7 +1295,7 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp,
> /* TODO: Make the format configurable. */
> info = drm_format_info(DRM_FORMAT_YUV422);
> zynqmp_disp_layer_set_format(layer, info);
> - zynqmp_disp_layer_enable(layer, ZYNQMP_DPSUB_LAYER_LIVE);
> + zynqmp_disp_layer_enable(layer);
>
> if (layer_id == ZYNQMP_DPSUB_LAYER_GFX)
> zynqmp_disp_blend_set_global_alpha(dp->dpsub->disp, true, 255);
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> index db3bb4afbfc4..43bf416b33d5 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> @@ -122,7 +122,7 @@ static void zynqmp_dpsub_plane_atomic_update(struct drm_plane *plane,
>
> /* Enable or re-enable the plane if the format has changed. */
> if (format_changed)
> - zynqmp_disp_layer_enable(layer, ZYNQMP_DPSUB_LAYER_NONLIVE);
> + zynqmp_disp_layer_enable(layer);
> }
>
> static const struct drm_plane_helper_funcs zynqmp_dpsub_plane_helper_funcs = {
>
^ permalink raw reply
* [PATCH v2 0/6] firmware: support i.MX95 SCMI BBM/MISC Extenstion
From: Peng Fan (OSS) @ 2024-04-05 12:39 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Sudeep Holla, Cristian Marussi
Cc: Peng Fan, devicetree, imx, linux-arm-kernel, linux-kernel
i.MX95 System Manager Firmware support vendor extension protocol:
- Battery Backed Module(BBM) Protocol for RTC and BUTTON feature.
- MISC Protocol for misc settings, such as BLK CTRL GPR settings, GPIO
expander settings.
This patchset is to support the two protocols and users that use the
protocols.
Signed-off-by: Peng Fan <peng.fan@nxp.com>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
To: Shawn Guo <shawnguo@kernel.org>
To: Sascha Hauer <s.hauer@pengutronix.de>
To: Pengutronix Kernel Team <kernel@pengutronix.de>
To: Fabio Estevam <festevam@gmail.com>
To: Peng Fan <peng.fan@nxp.com>
To: Sudeep Holla <sudeep.holla@arm.com>
To: Cristian Marussi <cristian.marussi@arm.com>
Cc: devicetree@vger.kernel.org
Cc: imx@lists.linux.dev
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Changes in v2:
- Sorry for late update since v1.
- Add a new patch 1
- Address imx,scmi.yaml issues
- Address comments for imx-sm-bbm.c and imx-sm-misc.c
- I not add vendor id since related patches not landed in linux-next.
- Link to v1: https://lore.kernel.org/r/20240202-imx95-bbm-misc-v1-0-3cb743020933@nxp.com
---
Peng Fan (6):
dt-bindings: firmware: arm,scmi: set additionalProperties to true
dt-bindings: firmware: add i.MX SCMI Extension protocol
firmware: arm_scmi: add initial support for i.MX BBM protocol
firmware: arm_scmi: add initial support for i.MX MISC protocol
firmware: imx: support BBM module
firmware: imx: add i.MX95 MISC driver
.../devicetree/bindings/firmware/arm,scmi.yaml | 2 +-
.../devicetree/bindings/firmware/imx,scmi.yaml | 80 +++++
drivers/firmware/arm_scmi/Kconfig | 20 ++
drivers/firmware/arm_scmi/Makefile | 2 +
drivers/firmware/arm_scmi/imx-sm-bbm.c | 378 +++++++++++++++++++++
drivers/firmware/arm_scmi/imx-sm-misc.c | 305 +++++++++++++++++
drivers/firmware/imx/Makefile | 2 +
drivers/firmware/imx/sm-bbm.c | 317 +++++++++++++++++
drivers/firmware/imx/sm-misc.c | 92 +++++
include/linux/firmware/imx/sm.h | 33 ++
include/linux/scmi_imx_protocol.h | 62 ++++
11 files changed, 1292 insertions(+), 1 deletion(-)
---
base-commit: 2b3d5988ae2cb5cd945ddbc653f0a71706231fdd
change-id: 20240405-imx95-bbm-misc-v2-b5e9d24adc42
Best regards,
--
Peng Fan <peng.fan@nxp.com>
^ permalink raw reply
* [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set additionalProperties to true
From: Peng Fan (OSS) @ 2024-04-05 12:39 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Sudeep Holla, Cristian Marussi
Cc: Peng Fan, devicetree, imx, linux-arm-kernel, linux-kernel
In-Reply-To: <20240405-imx95-bbm-misc-v2-v2-0-9fc9186856c2@nxp.com>
From: Peng Fan <peng.fan@nxp.com>
When adding vendor extension protocols, there is dt-schema warning:
"
imx,scmi.example.dtb: scmi: 'protocol@81', 'protocol@84' do not match any
of the regexes: 'pinctrl-[0-9]+'
"
Set additionalProperties to true to address the issue.
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 4591523b51a0..cfc613b65585 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -247,7 +247,7 @@ properties:
reg:
const: 0x18
-additionalProperties: false
+additionalProperties: true
$defs:
protocol-node:
--
2.37.1
^ permalink raw reply related
* [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI Extension protocol
From: Peng Fan (OSS) @ 2024-04-05 12:39 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Sudeep Holla, Cristian Marussi
Cc: Peng Fan, devicetree, imx, linux-arm-kernel, linux-kernel
In-Reply-To: <20240405-imx95-bbm-misc-v2-v2-0-9fc9186856c2@nxp.com>
From: Peng Fan <peng.fan@nxp.com>
Add i.MX SCMI Extension protocols bindings for:
- Battery Backed Secure Module(BBSM)
- MISC settings such as General Purpose Registers settings.
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
.../devicetree/bindings/firmware/imx,scmi.yaml | 80 ++++++++++++++++++++++
1 file changed, 80 insertions(+)
diff --git a/Documentation/devicetree/bindings/firmware/imx,scmi.yaml b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
new file mode 100644
index 000000000000..7ee19a661d83
--- /dev/null
+++ b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
@@ -0,0 +1,80 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2024 NXP
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/firmware/imx,scmi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: i.MX System Control and Management Interface(SCMI) Vendor Protocols Extension
+
+maintainers:
+ - Peng Fan <peng.fan@nxp.com>
+
+allOf:
+ - $ref: arm,scmi.yaml#
+
+properties:
+ protocol@81:
+ $ref: 'arm,scmi.yaml#/$defs/protocol-node'
+ unevaluatedProperties: false
+ description:
+ The BBM Protocol is for managing Battery Backed Secure Module (BBSM) RTC
+ and the ON/OFF Key
+
+ properties:
+ reg:
+ const: 0x81
+
+ required:
+ - reg
+
+ protocol@84:
+ $ref: 'arm,scmi.yaml#/$defs/protocol-node'
+ unevaluatedProperties: false
+ description:
+ The MISC Protocol is for managing SoC Misc settings, such as GPR settings
+
+ properties:
+ reg:
+ const: 0x84
+
+ wakeup-sources:
+ description:
+ Each entry consists of 2 integers, represents the source and electric signal edge
+ items:
+ items:
+ - description: the wakeup source
+ - description: the wakeup electric signal edge
+ $ref: /schemas/types.yaml#/definitions/uint32-matrix
+
+ required:
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ firmware {
+ scmi {
+ compatible = "arm,scmi";
+ mboxes = <&mu2 5 0>, <&mu2 3 0>, <&mu2 3 1>;
+ shmem = <&scmi_buf0>, <&scmi_buf1>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ protocol@81 {
+ reg = <0x81>;
+ };
+
+ protocol@84 {
+ reg = <0x84>;
+ wakeup-sources = <0x8000 1
+ 0x8001 1
+ 0x8002 1
+ 0x8003 1
+ 0x8004 1>;
+ };
+ };
+ };
+...
--
2.37.1
^ permalink raw reply related
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