* Re: [PATCH v3 1/2] dt-bindings: iio: imu: add icm42688 inside inv_icm42600
From: Krzysztof Kozlowski @ 2024-04-08 9:52 UTC (permalink / raw)
To: inv.git-commit, jic23, robh, krzysztof.kozlowski+dt, conor+dt
Cc: lars, linux-iio, devicetree, Jean-Baptiste Maneyrol
In-Reply-To: <20240408090720.847107-2-inv.git-commit@tdk.com>
On 08/04/2024 11:07, inv.git-commit@tdk.com wrote:
> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
>
> Add bindings for ICM-42688-P chip.
>
> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
> ---
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
This is an automated instruction, just in case, because many review tags
are being ignored. If you know the process, you can skip it (please do
not feel offended by me posting it here - no bad intentions intended).
If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions, under or above your Signed-off-by tag. Tag is "received", when
provided in a message replied to you on the mailing list. Tools like b4
can help here. However, there's no need to repost patches *only* to add
the tags. The upstream maintainer will do that for tags received on the
version they apply.
https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
Best regards,
Krzysztof
^ permalink raw reply
* Re: [RFC PATCH v2 1/5] clk: meson: axg: move reset controller's code to separate module
From: Stephen Boyd @ 2024-04-08 9:52 UTC (permalink / raw)
To: Jan Dakinevich, Jerome Brunet, Philipp Zabel
Cc: Neil Armstrong, Michael Turquette, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Kevin Hilman,
Martin Blumenstingl, linux-amlogic, linux-clk, devicetree,
linux-kernel, linux-arm-kernel
In-Reply-To: <f01cdd910ab35316b8012795f73fd2b34c8e6f8e.camel@pengutronix.de>
Quoting Philipp Zabel (2024-04-08 01:21:47)
> On So, 2024-04-07 at 19:39 -0700, Stephen Boyd wrote:
> > Quoting Jerome Brunet (2024-04-02 07:52:38)
> > >
> > > On Thu 28 Mar 2024 at 04:08, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote:
> > >
> > > > This code will by reused by A1 SoC.
> > >
> > > Could expand a bit please ?
> > >
> > > >
> > > > Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com>
> > >
> > > In general, I like the idea.
> > >
> > > We do have a couple a reset registers lost in middle of clocks and this
> > > change makes it possible to re-use the code instead duplicating it.
> > >
> > > The exported function would be used by audio clock controllers, but the
> > > module created would be purely about reset.
> > >
> > > One may wonder how it ended up in the clock tree, especially since the
> > > kernel as a reset tree too.
> > >
> > > I'm not sure if this should move to the reset framework or if it would
> > > be an unnecessary churn. Stephen, Philipp, do you have an opinion on
> > > this ?
> > >
> >
> > I'd prefer it be made into an auxiliary device and the driver put in
> > drivers/reset/ so we can keep reset code in the reset directory.
>
> Seconded, the clk-mpfs/reset-mpfs and clk-starfive-jh7110-sys/reset-
> starfive-jh7110 drivers are examples of this.
>
> > The auxiliary device creation function can also be in the
> > drivers/reset/ directory so that the clk driver calls some function
> > to create and register the device.
>
> I'm undecided about this, do you think mpfs_reset_controller_register()
> and jh7110_reset_controller_register() should rather live with the
> reset aux drivers in drivers/reset/ ?
Yes, and also mpfs_reset_read() and friends. We should pass the base
iomem pointer and parent device to mpfs_reset_adev_alloc() instead and
then move all that code into drivers/reset with some header file
exported function to call. That way the clk driver hands over the data
without having to implement half the implementation.
^ permalink raw reply
* Re: [PATCH v9 6/6] PCI: qcom: Add OPP support to scale performance state of power domain
From: Krishna Chaitanya Chundru @ 2024-04-08 9:52 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Lorenzo Pieralisi, Krzysztof Wilczyński,
Bjorn Helgaas, johan+linaro, bmasney, djakov, linux-arm-msm,
devicetree, linux-kernel, linux-pci, vireshk, quic_vbadigan,
quic_skananth, quic_nitegupt, quic_parass, krzysztof.kozlowski
In-Reply-To: <20240408094525.GB5727@thinkpad>
On 4/8/2024 3:15 PM, Manivannan Sadhasivam wrote:
> On Mon, Apr 08, 2024 at 02:32:18PM +0530, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 4/7/2024 8:30 PM, Manivannan Sadhasivam wrote:
>>> On Sun, Apr 07, 2024 at 10:07:39AM +0530, Krishna chaitanya chundru wrote:
>>>> QCOM Resource Power Manager-hardened (RPMh) is a hardware block which
>>>> maintains hardware state of a regulator by performing max aggregation of
>>>> the requests made by all of the clients.
>>>>
>>>> PCIe controller can operate on different RPMh performance state of power
>>>> domain based on the speed of the link. And this performance state varies
>>>> from target to target, like some controllers support GEN3 in NOM (Nominal)
>>>> voltage corner, while some other supports GEN3 in low SVS (static voltage
>>>> scaling).
>>>>
>>>> The SoC can be more power efficient if we scale the performance state
>>>> based on the aggregate PCIe link bandwidth.
>>>>
>>>> Add Operating Performance Points (OPP) support to vote for RPMh state based
>>>> on the aggregate link bandwidth.
>>>>
>>>> OPP can handle ICC bw voting also, so move ICC bw voting through OPP
>>>> framework if OPP entries are present.
>>>>
>>>> Different link configurations may share the same aggregate bandwidth,
>>>> e.g., a 2.5 GT/s x2 link and a 5.0 GT/s x1 link have the same bandwidth
>>>> and share the same OPP entry.
>>>>
>>>
>>> This info should be part of the dts change.
>>>
>> ok I will move this to dts patch in next patch series.
>>>> As we are moving ICC voting as part of OPP, don't initialize ICC if OPP
>>>> is supported.
>>>>
>>>> Before PCIe link is initialized vote for highest OPP in the OPP table,
>>>> so that we are voting for maximum voltage corner for the link to come up
>>>> in maximum supported speed.
>>>>
>>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>>>> ---
>>>> drivers/pci/controller/dwc/pcie-qcom.c | 72 +++++++++++++++++++++++++++-------
>>>> 1 file changed, 58 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>>>> index b4893214b2d3..4ad5ef3bf8fc 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>>> @@ -22,6 +22,7 @@
>>>> #include <linux/of.h>
>>>> #include <linux/of_gpio.h>
>>>> #include <linux/pci.h>
>>>> +#include <linux/pm_opp.h>
>>>> #include <linux/pm_runtime.h>
>>>> #include <linux/platform_device.h>
>>>> #include <linux/phy/pcie.h>
>>>> @@ -1442,15 +1443,13 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
>>>> return 0;
>>>> }
>>>> -static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
>>>> +static void qcom_pcie_icc_opp_update(struct qcom_pcie *pcie)
>>>> {
>>>> struct dw_pcie *pci = pcie->pci;
>>>> - u32 offset, status;
>>>> + u32 offset, status, freq;
>>>> + struct dev_pm_opp *opp;
>>>> int speed, width;
>>>> - int ret;
>>>> -
>>>> - if (!pcie->icc_mem)
>>>> - return;
>>>> + int ret, mbps;
>>>> offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>>>> status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
>>>> @@ -1462,10 +1461,26 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
>>>> speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
>>>> width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
>>>> - ret = icc_set_bw(pcie->icc_mem, 0, width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
>>>> - if (ret) {
>>>> - dev_err(pci->dev, "failed to set interconnect bandwidth for PCIe-MEM: %d\n",
>>>> - ret);
>>>> + if (pcie->icc_mem) {
>>>> + ret = icc_set_bw(pcie->icc_mem, 0, width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
>>>> + if (ret) {
>>>> + dev_err(pci->dev, "failed to set interconnect bandwidth for PCIe-MEM: %d\n",
>>>
>>> s/failed/Failed
>>>
>>>> + ret);
>>>> + }
>>>> + } else {
>>>> + mbps = pcie_link_speed_to_mbps(pcie_link_speed[speed]);
>>>> + if (mbps < 0)
>>>> + return;
>>>> +
>>>> + freq = mbps * 1000;
>>>> + opp = dev_pm_opp_find_freq_exact(pci->dev, freq * width, true);
>>>
>>> As per the API documentation, dev_pm_opp_put() should be called for both success
>>> and failure case.
>>>
>> ACK.
>>>> + if (!IS_ERR(opp)) {
>>>
>>> So what is the action if OPP is not found for the freq?
>>>
>> There is already a vote for maximum freq in the probe, so if it fails
>> here we can continue here.
>> If you feel otherwise let me know I Can make changes as suggested.
>
> You should just log the error and continue.
>
>>>> + ret = dev_pm_opp_set_opp(pci->dev, opp);
>>>> + if (ret)
>>>> + dev_err(pci->dev, "Failed to set opp: freq %ld ret %d\n",
>>>
>>> 'Failed to set OPP for freq (%ld): %d'
>>>
>>>> + dev_pm_opp_get_freq(opp), ret);
>>>> + dev_pm_opp_put(opp);
>>>> + }
>>>> }
>>>> }
>>>> @@ -1509,8 +1524,10 @@ static void qcom_pcie_init_debugfs(struct qcom_pcie *pcie)
>>>> static int qcom_pcie_probe(struct platform_device *pdev)
>>>> {
>>>> const struct qcom_pcie_cfg *pcie_cfg;
>>>> + unsigned long max_freq = INT_MAX;
>>>> struct device *dev = &pdev->dev;
>>>> struct qcom_pcie *pcie;
>>>> + struct dev_pm_opp *opp;
>>>> struct dw_pcie_rp *pp;
>>>> struct resource *res;
>>>> struct dw_pcie *pci;
>>>> @@ -1577,9 +1594,33 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>>> goto err_pm_runtime_put;
>>>> }
>>>> - ret = qcom_pcie_icc_init(pcie);
>>>> - if (ret)
>>>> + /* OPP table is optional */
>>>> + ret = devm_pm_opp_of_add_table(dev);
>>>> + if (ret && ret != -ENODEV) {
>>>> + dev_err_probe(dev, ret, "Failed to add OPP table\n");
>>>> goto err_pm_runtime_put;
>>>> + }
>>>> +
>>>> + /*
>>>> + * Use highest OPP here if the OPP table is present. At the end of
>>>
>>> I believe I asked you to add the information justifying why the highest OPP
>>> should be used.
>>>
>> I added the info in the commit message, I will add as the comment in the
>> next patch.
>>
>>>> + * the probe(), OPP will be updated using qcom_pcie_icc_opp_update().
>>>> + */
>>>> + if (!ret) {
>>>> + opp = dev_pm_opp_find_freq_floor(dev, &max_freq);
>>>
>>> Same comment as dev_pm_opp_find_freq_exact().
>>>
>>>> + if (!IS_ERR(opp)) {
>>>> + ret = dev_pm_opp_set_opp(dev, opp);
>>>> + if (ret)
>>>> + dev_err_probe(pci->dev, ret,
>>>> + "Failed to set OPP: freq %ld\n",
>>>
>>> Same comment as above.
>>>
>>>> + dev_pm_opp_get_freq(opp));
>>>> + dev_pm_opp_put(opp);
>>>
>>> So you want to continue even in the case of failure?
>>>
>> I wil make changes to fallback to driver voting for icc bw if it fails here.
>
> That's not needed. If the OPP table is present, then failure to set OPP should
> be treated as a hard failure.
>
Sure, I will make changes to fail the probe then
- Krishna Chaitanya.
> - Mani
>
^ permalink raw reply
* Re: [PATCH v8 4/4] pinctrl: Implementation of the generic scmi-pinctrl driver
From: Dhruva Gole @ 2024-04-08 9:52 UTC (permalink / raw)
To: Peng Fan (OSS)
Cc: Sudeep Holla, Cristian Marussi, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Linus Walleij, Dan Carpenter, Andy Shevchenko,
linux-arm-kernel, linux-kernel, devicetree, linux-gpio, Peng Fan,
Oleksii Moisieiev, Tony Lindgren, Kevin Hilman,
Vignesh Raghavendra
In-Reply-To: <20240405-pinctrl-scmi-v8-4-5fc8e33871bf@nxp.com>
On Apr 05, 2024 at 09:59:35 +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> scmi-pinctrl driver implements pinctrl driver interface and using
> SCMI protocol to redirect messages from pinctrl subsystem SDK to
> SCMI platform firmware, which does the changes in HW.
>
> Co-developed-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> MAINTAINERS | 1 +
> drivers/pinctrl/Kconfig | 11 +
> drivers/pinctrl/Makefile | 1 +
> drivers/pinctrl/pinctrl-scmi.c | 564 +++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 577 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4b511a55101c..d8270ac6651a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21457,6 +21457,7 @@ F: drivers/cpufreq/sc[mp]i-cpufreq.c
> F: drivers/firmware/arm_scmi/
> F: drivers/firmware/arm_scpi.c
> F: drivers/hwmon/scmi-hwmon.c
> +F: drivers/pinctrl/pinctrl-scmi.c
> F: drivers/pmdomain/arm/
> F: drivers/powercap/arm_scmi_powercap.c
> F: drivers/regulator/scmi-regulator.c
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index d45657aa986a..4e6f65cf0e76 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -450,6 +450,17 @@ config PINCTRL_ROCKCHIP
> help
> This support pinctrl and GPIO driver for Rockchip SoCs.
>
> +config PINCTRL_SCMI
> + tristate "Pinctrl driver using SCMI protocol interface"
> + depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
> + select PINMUX
> + select GENERIC_PINCONF
> + help
> + This driver provides support for pinctrl which is controlled
> + by firmware that implements the SCMI interface.
> + It uses SCMI Message Protocol to interact with the
> + firmware providing all the pinctrl controls.
> +
> config PINCTRL_SINGLE
> tristate "One-register-per-pin type device tree based pinctrl driver"
> depends on OF
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> index 2152539b53d5..cc809669405a 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -45,6 +45,7 @@ obj-$(CONFIG_PINCTRL_PIC32) += pinctrl-pic32.o
> obj-$(CONFIG_PINCTRL_PISTACHIO) += pinctrl-pistachio.o
> obj-$(CONFIG_PINCTRL_RK805) += pinctrl-rk805.o
> obj-$(CONFIG_PINCTRL_ROCKCHIP) += pinctrl-rockchip.o
> +obj-$(CONFIG_PINCTRL_SCMI) += pinctrl-scmi.o
> obj-$(CONFIG_PINCTRL_SINGLE) += pinctrl-single.o
> obj-$(CONFIG_PINCTRL_ST) += pinctrl-st.o
> obj-$(CONFIG_PINCTRL_STMFX) += pinctrl-stmfx.o
> diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
> new file mode 100644
> index 000000000000..0f55f000a679
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-scmi.c
> @@ -0,0 +1,564 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Power Interface (SCMI) Protocol based pinctrl driver
> + *
> + * Copyright (C) 2024 EPAM
> + * Copyright 2024 NXP
> + */
> +
> +#include <linux/device.h>
> +#include <linux/dev_printk.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +#include <linux/pinctrl/machine.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +
> +#include "pinctrl-utils.h"
> +#include "core.h"
> +#include "pinconf.h"
> +
> +#define DRV_NAME "scmi-pinctrl"
> +
> +/* Define num configs, if not large than 4 use stack, else use kcalloc */
> +#define SCMI_NUM_CONFIGS 4
> +
> +static const struct scmi_pinctrl_proto_ops *pinctrl_ops;
> +
> +struct scmi_pinctrl {
> + struct device *dev;
> + struct scmi_protocol_handle *ph;
> + struct pinctrl_dev *pctldev;
> + struct pinctrl_desc pctl_desc;
> + struct pinfunction *functions;
> + unsigned int nr_functions;
> + struct pinctrl_pin_desc *pins;
> + unsigned int nr_pins;
> +};
> +
> +static int pinctrl_scmi_get_groups_count(struct pinctrl_dev *pctldev)
> +{
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + return pinctrl_ops->count_get(pmx->ph, GROUP_TYPE);
> +}
> +
> +static const char *pinctrl_scmi_get_group_name(struct pinctrl_dev *pctldev,
> + unsigned int selector)
> +{
> + int ret;
> + const char *name;
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + ret = pinctrl_ops->name_get(pmx->ph, selector, GROUP_TYPE, &name);
> + if (ret) {
> + dev_err(pmx->dev, "get name failed with err %d", ret);
> + return NULL;
> + }
> +
> + return name;
> +}
> +
> +static int pinctrl_scmi_get_group_pins(struct pinctrl_dev *pctldev,
> + unsigned int selector,
> + const unsigned int **pins,
> + unsigned int *num_pins)
> +{
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + return pinctrl_ops->group_pins_get(pmx->ph, selector, pins, num_pins);
> +}
> +
> +static const struct pinctrl_ops pinctrl_scmi_pinctrl_ops = {
> + .get_groups_count = pinctrl_scmi_get_groups_count,
> + .get_group_name = pinctrl_scmi_get_group_name,
> + .get_group_pins = pinctrl_scmi_get_group_pins,
> +#ifdef CONFIG_OF
> + .dt_node_to_map = pinconf_generic_dt_node_to_map_all,
> + .dt_free_map = pinconf_generic_dt_free_map,
> +#endif
> +};
> +
> +static int pinctrl_scmi_get_functions_count(struct pinctrl_dev *pctldev)
> +{
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + return pinctrl_ops->count_get(pmx->ph, FUNCTION_TYPE);
> +}
> +
> +static const char *pinctrl_scmi_get_function_name(struct pinctrl_dev *pctldev,
> + unsigned int selector)
> +{
> + int ret;
> + const char *name;
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + ret = pinctrl_ops->name_get(pmx->ph, selector, FUNCTION_TYPE, &name);
> + if (ret) {
> + dev_err(pmx->dev, "get name failed with err %d", ret);
> + return NULL;
> + }
> +
> + return name;
> +}
> +
> +static int pinctrl_scmi_get_function_groups(struct pinctrl_dev *pctldev,
> + unsigned int selector,
> + const char * const **p_groups,
> + unsigned int * const p_num_groups)
> +{
> + struct pinfunction *func;
> + const unsigned int *group_ids;
> + unsigned int num_groups;
> + const char **groups;
> + int ret, i;
Just a nit maybe, but I would be more comfortable making i with
num_groups as unsigned, because you're comparing them after all in the
loop. Also, I don't see a reason for i to become negative in any case.
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!p_groups || !p_num_groups)
> + return -EINVAL;
> +
> + if (selector >= pmx->nr_functions)
> + return -EINVAL;
> +
> + func = &pmx->functions[selector];
> + if (func->ngroups)
> + goto done;
> +
> + ret = pinctrl_ops->function_groups_get(pmx->ph, selector, &num_groups,
> + &group_ids);
> + if (ret) {
> + dev_err(pmx->dev, "Unable to get function groups, err %d", ret);
> + return ret;
> + }
> + if (!num_groups)
> + return -EINVAL;
> +
> + groups = kcalloc(num_groups, sizeof(*groups), GFP_KERNEL);
> + if (!groups)
> + return -ENOMEM;
> +
> + for (i = 0; i < num_groups; i++) {
> + groups[i] = pinctrl_scmi_get_group_name(pctldev, group_ids[i]);
> + if (!groups[i]) {
> + ret = -EINVAL;
> + goto err_free;
> + }
> + }
> +
> + func->ngroups = num_groups;
> + func->groups = groups;
> +done:
> + *p_groups = func->groups;
> + *p_num_groups = func->ngroups;
> +
> + return 0;
> +
> +err_free:
> + kfree(groups);
> +
> + return ret;
> +}
> +
[...]
> +
> +static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
> + struct pinctrl_desc *desc)
> +{
> + struct pinctrl_pin_desc *pins;
> + unsigned int npins;
> + int ret, i;
better unsigned i?
> +
> + npins = pinctrl_ops->count_get(pmx->ph, PIN_TYPE);
> + /*
> + * npins will never be zero, the scmi pinctrl driver has bailed out
> + * if npins is zero.
> + */
> + pins = devm_kmalloc_array(pmx->dev, npins, sizeof(*pins), GFP_KERNEL);
> + if (!pins)
> + return -ENOMEM;
> +
> + for (i = 0; i < npins; i++) {
> + pins[i].number = i;
> + /*
> + * The memory for name is handled by the scmi firmware driver,
> + * no need free here
> + */
> + ret = pinctrl_ops->name_get(pmx->ph, i, PIN_TYPE, &pins[i].name);
> + if (ret)
> + return dev_err_probe(pmx->dev, ret,
> + "Can't get name for pin %d", i);
> + }
> +
> + desc->npins = npins;
> + desc->pins = pins;
> + dev_dbg(pmx->dev, "got pins %u", npins);
> +
> + return 0;
> +}
> +
[...]
Unrelated and beyond scope of this patch series, but would've loved to
see concept of wakeup enable and wakeup event bits inside the pinctrl
SCMI spec like we have in pinctrl-single kernel driver. There are SOC's
out there that support wakeup IRQ's from their padconfig controllers
itself... But this is more of a feedback for the SCMI spec. Maybe a
future revision can take care of this.
The reason this needs to be
standard and not something vendor specific is because the kernel does
support a wake IRQ framework, and we will need to make this driver have
wake IRQ support if a device that supports pinctrl wakeup need to use
scmi to configure it.
Look at Table 6-2045. Description Of The Pad Configuration Register Bit
in [0] for further details for an example of a padconfig wakeup config
specially bits 29,30.
No major comments otherwise,
Reviewed-by: Dhruva Gole <d-gole@ti.com>
[0] https://www.ti.com/lit/pdf/SPRUIV7
--
Best regards,
Dhruva Gole <d-gole@ti.com>
^ permalink raw reply
* [PATCH] arm64: dts: mediatek: mt7622: fix "emmc" pinctrl mux
From: Rafał Miłecki @ 2024-04-08 10:00 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, Matthias Brugger
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Sean Wang,
Ryder Lee, devicetree, linux-arm-kernel, linux-mediatek,
Rafał Miłecki
From: Rafał Miłecki <rafal@milecki.pl>
Value "emmc_rst" is a group name and should be part of the "groups"
property.
This fixes:
arch/arm64/boot/dts/mediatek/mt7622-rfb1.dtb: pinctrl@10211000: emmc-pins-default:mux:function: ['emmc', 'emmc_rst'] is too long
from schema $id: http://devicetree.org/schemas/pinctrl/mediatek,mt7622-pinctrl.yaml#
arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dtb: pinctrl@10211000: emmc-pins-default:mux:function: ['emmc', 'emmc_rst'] is too long
from schema $id: http://devicetree.org/schemas/pinctrl/mediatek,mt7622-pinctrl.yaml#
Fixes: 3725ba3f5574 ("arm64: dts: mt7622: add pinctrl related device nodes")
Fixes: 0b6286dd96c0 ("arm64: dts: mt7622: add bananapi BPI-R64 board")
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts | 4 ++--
arch/arm64/boot/dts/mediatek/mt7622-rfb1.dts | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts b/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts
index 224bb289660c..2791de5b28f6 100644
--- a/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts
+++ b/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts
@@ -329,8 +329,8 @@ asm_sel {
/* eMMC is shared pin with parallel NAND */
emmc_pins_default: emmc-pins-default {
mux {
- function = "emmc", "emmc_rst";
- groups = "emmc";
+ function = "emmc";
+ groups = "emmc", "emmc_rst";
};
/* "NDL0","NDL1","NDL2","NDL3","NDL4","NDL5","NDL6","NDL7",
diff --git a/arch/arm64/boot/dts/mediatek/mt7622-rfb1.dts b/arch/arm64/boot/dts/mediatek/mt7622-rfb1.dts
index 41629769bdc8..8c3e2e2578bc 100644
--- a/arch/arm64/boot/dts/mediatek/mt7622-rfb1.dts
+++ b/arch/arm64/boot/dts/mediatek/mt7622-rfb1.dts
@@ -268,8 +268,8 @@ &pio {
/* eMMC is shared pin with parallel NAND */
emmc_pins_default: emmc-pins-default {
mux {
- function = "emmc", "emmc_rst";
- groups = "emmc";
+ function = "emmc";
+ groups = "emmc", "emmc_rst";
};
/* "NDL0","NDL1","NDL2","NDL3","NDL4","NDL5","NDL6","NDL7",
--
2.35.3
^ permalink raw reply related
* Re: [PATCH v3 2/2] mailbox: arm_mhuv3: Add driver
From: Cristian Marussi @ 2024-04-08 10:08 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-kernel, linux-arm-kernel, devicetree, sudeep.holla,
jassisinghbrar, robh+dt, krzysztof.kozlowski+dt, conor+dt
In-Reply-To: <20240405113200.00002da9@Huawei.com>
On Fri, Apr 05, 2024 at 11:32:00AM +0100, Jonathan Cameron wrote:
> 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 :)
>
You're welcome...thanks for having a look !
>
> > 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?
>
Yes, it is indeed cryptic, these are the holes in the MMIO regs and
are derived from the spec...I'll see how I can rework to make this more
meaningful and better commented.
> > + 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?
Yes, all the struct/bitfield based MMIO stuff is borrowed from mhuv2
since it seemed more slick than a zillions defines and bitmasks (but maybe
not exempt from issues... given what Jassi jas commented later on...), BUT
while using those I saw the opportunity to drop a parameter since v2 has a
_type arg that it can indeed derived from _regptr, so making the macros less
cumbersome to invoke....THEN sparse quicly reminded me that by using typeof()
to derive the type of the local work-variable I was also grabbing all the
related attributes attached to _regptr...namely __iomem and noderef that
triggered a bunch of warnings (unjustified since operating on a local
var NOT a real MMIO): that is the reason for the dance with __force
here, and why is not needed in v2.
> > +
> > +/* ====== 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.
>
Indeed.
> > + DBE_EXT = FIRST_EXT,
> > + FCE_EXT,
> > + FE_EXT,
> > + MAX_EXT
> That's one past normal meeting of MAX, maybe call it COUNT, or NUM?
>
Ok.
> > +};
>
> > +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.
>
Yes, I'll have a look into cleanup.h at large.
>
> > + /* 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
>
Ok.
> > + 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.
>
Yes.
> > + 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.
>
Ok.
> > +
> > + 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.
>
Ok, I'll do...I was thinking was more clear to specify what was the
start instead of a plain 0....but indeed is apparent from the context.
> > + 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)
>
Yes, indeed. I've just posted a series to use dev_err_probe on the SCMI
stack and then missed completely here...my bad. I'll do.
> > + }
> > +
> > + 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?
>
Right.
> > + 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.
Yes, I'll fix the logic.
>
> > +
> > + 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.
>
Ok.
> > + 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.
>
ok.
> > + 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.
>
I will give a go at reworking.
> > +
> > + 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.
Well, the only reason is that when the Tx-Ack interrupt fails somehow to
be setup (or is not provided even though the spec mandates it), the
mailbox can work anyway fine, maybe on degraded performance...so here I was
trying to be kind and carry-on best-effort with a few warnings...since
I already bumped into a system where the Tx-Ack was supposedly present BUT
the wire was NOT ... but indeed better to be noisy and bailout so to have
the thing fixed early on when it happens...I'll revisit
>
>
> > + }
> > +
> > + dev_info(dev, "Using PBX in Tx polling mode.\n");
>
> That's noisy. dev_dbg() perhaps?
>
Ok, I was trying to be noisy indeed since operating without Tx-Ack can
be limiting in some circumstances and is a sort of anomaly given the
spec would expect the PBX combined interrupt to be provided (but the
mailboxes can work...)
> > + 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.
>
Yes I'll switch to dev_err_probe where appropriate.
> > + 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()
Ditto.
>
> > + }
> > +
> > + 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.
Ok.
>
> > + } 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");
Ditto.
>
> return 0;
> > +
> > + platform_set_drvdata(pdev, mhu);
>
> With all devm as suggested below, can I think drop this.
>
Ok.
> > +
> > + 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.
Ok, I'll switch to devm_ fully and drop remove() all along.
Thanks again for the review.
Cristian
^ permalink raw reply
* Re: [PATCH] arm64: dts: imx8mp: Align both CSI2 pixel clock
From: Alexander Stein @ 2024-04-08 10:14 UTC (permalink / raw)
To: Adam Ford, Laurent Pinchart, linux-arm-kernel
Cc: linux-arm-kernel, Conor Dooley, Fabio Estevam,
Krzysztof Kozlowski, Paul Elder, Pengutronix Kernel Team,
Rob Herring, Sascha Hauer, Shawn Guo, devicetree, imx,
Marek Vasut
In-Reply-To: <8c1935d1-7f59-4742-9659-bf87ac4b736c@denx.de>
Hi everyone,
Am Samstag, 6. April 2024, 04:58:39 CEST schrieb Marek Vasut:
> On 4/5/24 11:04 PM, Adam Ford wrote:
> > On Fri, Apr 5, 2024 at 3:43 PM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> >>
> >> Hi Marek,
> >>
> >> (CC'ing Adam)
> >>
> >> Thank you for the patch.
> >>
> >> On Fri, Apr 05, 2024 at 10:22:26PM +0200, Marek Vasut wrote:
> >>> Configure both CSI2 assigned-clock-rates the same way.
> >>> There does not seem to be any reason for keeping the
> >>> two CSI2 pixel clock set to different frequencies.
> >>
> >> There's an issue when using two cameras concurrently. This has been
> >> discussed some time ago on the linux-media mailing list, see [1]. Adam
> >> knows more than I do on this topic.
> >>
> >> [1] https://lore.kernel.org/linux-media/CAHCN7x+kymRGO2kxvN2=zLiqRjfTc3hdf3VdNVkWjsW3La0bnA@mail.gmail.com/
> >>
> >>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>> ---
> >>> Cc: Conor Dooley <conor+dt@kernel.org>
> >>> Cc: Fabio Estevam <festevam@gmail.com>
> >>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> >>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> Cc: Paul Elder <paul.elder@ideasonboard.com>
> >>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> >>> Cc: Rob Herring <robh@kernel.org>
> >>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> >>> Cc: Shawn Guo <shawnguo@kernel.org>
> >>> Cc: devicetree@vger.kernel.org
> >>> Cc: imx@lists.linux.dev
> >>> Cc: linux-arm-kernel@lists.infradead.org
> >>> ---
> >>> arch/arm64/boot/dts/freescale/imx8mp.dtsi | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> >>> index 1bb96e96639f2..2e9ce0c3a9815 100644
> >>> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> >>> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> >>> @@ -1703,7 +1703,7 @@ mipi_csi_1: csi@32e50000 {
> >>> <&clk IMX8MP_CLK_MEDIA_MIPI_PHY1_REF>;
> >>> assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>,
> >>> <&clk IMX8MP_CLK_24M>;
> >>> - assigned-clock-rates = <266000000>;
> >>> + assigned-clock-rates = <500000000>;
> >
> > I am traveling, so I don't have the technical documents in front of
> > me, but I beleive this is an over-drive speed, and 400MHz would be the
> > single clock, standard rate. I created an imx8mm-overdrive and
> > imx8mn-overdrive dtsi file to let users who operate in overdrive mode
> > to update their clocks in one place.
> >
> > I also think this goes down if the user is running two cameras instead
> > of one. I re-read the old thread, and it's coming back to me, but
> > until I can get settled into my hotel in Germany, I won't have time to
> > review. I think the original idea was to use the lowest, conservative
> > value with the idea that people can tweak their clock settings if
> > they're only running one and if they are running in over-drive mode.
>
> MX8MPCEC does indeed read 400 MHz regular, 500 MHz overdrive.
>
> Shall we align both CSI2 ports to 400 MHz ? Currently they are one 500
> MHz and the other 266 MHz .
No, that won't do. The (industrial products) datasheet says (Table 1):
* Single camera on CSI1: 400/500 MHz in normal/overdrive mode
* Single camera on CSI2: 277 MHz
* Dual camera on CSI1 & CSI2: 266 MHz
Assuming you need CSI2 more likely in a dual camera setup only, defaulting to
266MHz seems sensible to me.
Best regards,
Alexander
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/
^ permalink raw reply
* Re: [PATCH v8 2/4] dt-bindings: firmware: arm,scmi: support pinctrl protocol
From: Dhruva Gole @ 2024-04-08 10:14 UTC (permalink / raw)
To: Peng Fan (OSS)
Cc: Sudeep Holla, Cristian Marussi, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Linus Walleij, Dan Carpenter, Andy Shevchenko,
linux-arm-kernel, linux-kernel, devicetree, linux-gpio, Peng Fan
In-Reply-To: <20240405-pinctrl-scmi-v8-2-5fc8e33871bf@nxp.com>
On Apr 05, 2024 at 09:59:33 +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> Add SCMI v3.2 pinctrl protocol bindings and example.
>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
Reviewed-by: Dhruva Gole <d-gole@ti.com>
> .../devicetree/bindings/firmware/arm,scmi.yaml | 50 ++++++++++++++++++++++
> 1 file changed, 50 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index 4591523b51a0..e9d3f043c4ed 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -247,6 +247,37 @@ properties:
> reg:
[...]
--
Best regards,
Dhruva
^ permalink raw reply
* Re: [PATCH v1 2/3] dt-bindings: arm: mediatek: mmsys: Add OF graph support for board path
From: AngeloGioacchino Del Regno @ 2024-04-08 10:16 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: chunkuang.hu, robh, krzysztof.kozlowski+dt, conor+dt, p.zabel,
airlied, daniel, maarten.lankhorst, mripard, tzimmermann,
matthias.bgg, shawn.sung, yu-chang.lee, ck.hu, jitao.shi,
devicetree, linux-kernel, dri-devel, linux-mediatek,
linux-arm-kernel, kernel
In-Reply-To: <CAGXv+5F9rfTVDExKSCF7fBKwR+HijNzFYE6+4aHKw3ZP81DG9w@mail.gmail.com>
Il 08/04/24 05:20, Chen-Yu Tsai ha scritto:
> On Thu, Apr 4, 2024 at 4:16 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Document OF graph on MMSYS/VDOSYS: this supports up to three DDP paths
>> per HW instance (so potentially up to six displays for multi-vdo SoCs).
>>
>> The MMSYS or VDOSYS is always the first component in the DDP pipeline,
>> so it only supports an output port with multiple endpoints - where each
>> endpoint defines the starting point for one of the (currently three)
>> possible hardware paths.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>> .../bindings/arm/mediatek/mediatek,mmsys.yaml | 23 +++++++++++++++++++
>> 1 file changed, 23 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
>> index b3c6888c1457..90758bb5bcb1 100644
>> --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
>> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
>> @@ -93,6 +93,29 @@ properties:
>> '#reset-cells':
>> const: 1
>>
>> + port:
>> + $ref: /schemas/graph.yaml#/properties/port
>> + description:
>> + Output port node. This port connects the MMSYS/VDOSYS output to
>> + the first component of one display pipeline, for example one of
>> + the available OVL or RDMA blocks.
>> + Some MediaTek SoCs support up to three display outputs per MMSYS.
>> + properties:
>> + endpoint@0:
>> + $ref: /schemas/graph.yaml#/properties/endpoint
>> + description: Output to the primary display pipeline
>> +
>> + endpoint@1:
>> + $ref: /schemas/graph.yaml#/properties/endpoint
>> + description: Output to the secondary display pipeline
>> +
>> + endpoint@2:
>> + $ref: /schemas/graph.yaml#/properties/endpoint
>> + description: Output to the tertiary display pipeline
>> +
>> + required:
>> + - endpoint@0
>> +
>
> Technically the mmsys device serves as an glue layer for the display
> pipeline, providing things like clock control and signal routing; the
> device itself is not part of the pipeline, and probably shouldn't be
> part of the graph?
>
That is (only) partially true: in the case of older SoCs, the MMSYS can only
connect to a single first IP of the pipeline, but in the case of newer ones,
and especially (but not limited to) MT8195 onwards having multiple instances
of VDOSYS, that really becomes part of the pipeline.
This is not because of the possible different first IP in the pipeline, but
because of support for dual-interface (DSI and DP) that, in even newer SoCs,
can be done with cross-mmsys (cross-vdosys, actually...) as some of those do
have the two in different VDOs.
So yes, this can be done without the graph in MMSYS *in this precise moment in
time*, but we'll anyway end up adding it sooner than later - and I'm doing this
right now, instead of later, because it's also simplifying the implementation
so like that I'm "catching two birds with one stone" :-)
Cheers,
Angelo
> ChenYu
>
>> required:
>> - compatible
>> - reg
>> --
>> 2.44.0
>>
^ permalink raw reply
* [PATCH v3 2/2] iio: imu: inv_icm42600: add support of ICM-42688-P
From: inv.git-commit @ 2024-04-08 9:07 UTC (permalink / raw)
To: jic23, robh, krzysztof.kozlowski+dt, conor+dt
Cc: lars, linux-iio, devicetree, Jean-Baptiste Maneyrol
In-Reply-To: <20240408090720.847107-1-inv.git-commit@tdk.com>
From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
Add ICM-42688-P support inside driver.
Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
---
drivers/iio/imu/inv_icm42600/inv_icm42600.h | 2 ++
drivers/iio/imu/inv_icm42600/inv_icm42600_core.c | 5 +++++
drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c | 3 +++
drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c | 3 +++
4 files changed, 13 insertions(+)
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
index 0e290c807b0f..0566340b2660 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
@@ -22,6 +22,7 @@ enum inv_icm42600_chip {
INV_CHIP_ICM42602,
INV_CHIP_ICM42605,
INV_CHIP_ICM42622,
+ INV_CHIP_ICM42688,
INV_CHIP_ICM42631,
INV_CHIP_NB,
};
@@ -304,6 +305,7 @@ struct inv_icm42600_state {
#define INV_ICM42600_WHOAMI_ICM42602 0x41
#define INV_ICM42600_WHOAMI_ICM42605 0x42
#define INV_ICM42600_WHOAMI_ICM42622 0x46
+#define INV_ICM42600_WHOAMI_ICM42688 0x47
#define INV_ICM42600_WHOAMI_ICM42631 0x5C
/* User bank 1 (MSB 0x10) */
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
index a5e81906e37e..82e0a2e2ad70 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
@@ -87,6 +87,11 @@ static const struct inv_icm42600_hw inv_icm42600_hw[INV_CHIP_NB] = {
.name = "icm42622",
.conf = &inv_icm42600_default_conf,
},
+ [INV_CHIP_ICM42688] = {
+ .whoami = INV_ICM42600_WHOAMI_ICM42688,
+ .name = "icm42688",
+ .conf = &inv_icm42600_default_conf,
+ },
[INV_CHIP_ICM42631] = {
.whoami = INV_ICM42600_WHOAMI_ICM42631,
.name = "icm42631",
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
index 1af559403ba6..ebb28f84ba98 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
@@ -84,6 +84,9 @@ static const struct of_device_id inv_icm42600_of_matches[] = {
}, {
.compatible = "invensense,icm42622",
.data = (void *)INV_CHIP_ICM42622,
+ }, {
+ .compatible = "invensense,icm42688",
+ .data = (void *)INV_CHIP_ICM42688,
}, {
.compatible = "invensense,icm42631",
.data = (void *)INV_CHIP_ICM42631,
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c
index 6be4ac794937..50217a10e0bb 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c
@@ -80,6 +80,9 @@ static const struct of_device_id inv_icm42600_of_matches[] = {
}, {
.compatible = "invensense,icm42622",
.data = (void *)INV_CHIP_ICM42622,
+ }, {
+ .compatible = "invensense,icm42688",
+ .data = (void *)INV_CHIP_ICM42688,
}, {
.compatible = "invensense,icm42631",
.data = (void *)INV_CHIP_ICM42631,
--
2.34.1
^ permalink raw reply related
* [PATCH v3 1/2] dt-bindings: iio: imu: add icm42688 inside inv_icm42600
From: inv.git-commit @ 2024-04-08 9:07 UTC (permalink / raw)
To: jic23, robh, krzysztof.kozlowski+dt, conor+dt
Cc: lars, linux-iio, devicetree, Jean-Baptiste Maneyrol
In-Reply-To: <20240408090720.847107-1-inv.git-commit@tdk.com>
From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
Add bindings for ICM-42688-P chip.
Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
---
.../devicetree/bindings/iio/imu/invensense,icm42600.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/iio/imu/invensense,icm42600.yaml b/Documentation/devicetree/bindings/iio/imu/invensense,icm42600.yaml
index 7cd05bcbee31..5e0bed2c45de 100644
--- a/Documentation/devicetree/bindings/iio/imu/invensense,icm42600.yaml
+++ b/Documentation/devicetree/bindings/iio/imu/invensense,icm42600.yaml
@@ -32,6 +32,7 @@ properties:
- invensense,icm42605
- invensense,icm42622
- invensense,icm42631
+ - invensense,icm42688
reg:
maxItems: 1
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v8 1/4] firmware: arm_scmi: introduce helper get_max_msg_size
From: Dhruva Gole @ 2024-04-08 10:22 UTC (permalink / raw)
To: Peng Fan (OSS)
Cc: Sudeep Holla, Cristian Marussi, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Linus Walleij, Dan Carpenter, Andy Shevchenko,
linux-arm-kernel, linux-kernel, devicetree, linux-gpio, Peng Fan
In-Reply-To: <20240405-pinctrl-scmi-v8-1-5fc8e33871bf@nxp.com>
On Apr 05, 2024 at 09:59:32 +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> When Agent sending data to SCMI server, the Agent driver could check
> the size to avoid protocol buffer overflow. So introduce the helper
> get_max_msg_size.
>
> Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> drivers/firmware/arm_scmi/driver.c | 15 +++++++++++++++
> drivers/firmware/arm_scmi/protocols.h | 2 ++
> 2 files changed, 17 insertions(+)
[...]
Reviewed-by: Dhruva Gole <d-gole@ti.com>
--
Best regards,
Dhruva
^ permalink raw reply
* Re: [PATCH] arm64: dts: renesas: rzg3s-smarc-som: Fix ethernet aliases
From: Geert Uytterhoeven @ 2024-04-08 10:24 UTC (permalink / raw)
To: Claudiu; +Cc: magnus.damm, robh+dt, linux-renesas-soc, devicetree, linux-kernel
In-Reply-To: <20240328065738.3049316-1-claudiu.beznea.uj@bp.renesas.com>
On Thu, Mar 28, 2024 at 7:57 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Fix typo in ethernet aliases. U-Boot uses ethernetX (X={0, 1, ..., N})
> aliases to update the DTB of Linux with MAC addresses. The ethernetX or
> ethX aliases are not used in Linux by ravb_driver.
>
> Fixes: 932ff0c802c6 ("arm64: dts: renesas: rzg3s-smarc-som: Enable the Ethernet interfaces")
> Suggested-by: Biju Das <biju.das.jz@bp.renesas.com>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-devel for v6.10.
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 v3 2/2] mailbox: arm_mhuv3: Add driver
From: Cristian Marussi @ 2024-04-08 10:40 UTC (permalink / raw)
To: Jassi Brar
Cc: linux-kernel, linux-arm-kernel, devicetree, sudeep.holla, robh+dt,
krzysztof.kozlowski+dt, conor+dt
In-Reply-To: <CABb+yY3yzj167ypLPPV7OvqDWfv9y9EFQeZ5pxPAx7xUyyvLVw@mail.gmail.com>
On Sun, Apr 07, 2024 at 08:14:23PM -0500, Jassi Brar wrote:
> On Thu, Apr 4, 2024 at 1:25 AM 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.
> >
Hi Jassi,
thanks for having a look at this !
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> > v1 -> v2
> > - fixed checkpatch warnings about side-effects
> > - fixed sparse errors as reported
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202403290015.tCLXudqC-lkp@intel.com/
> > ---
> > MAINTAINERS | 9 +
> > drivers/mailbox/Kconfig | 11 +
> > drivers/mailbox/Makefile | 2 +
> > drivers/mailbox/arm_mhuv3.c | 1063 +++++++++++++++++++++++++++++++++++
> > 4 files changed, 1085 insertions(+)
> > create mode 100644 drivers/mailbox/arm_mhuv3.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index aa3b947fb080..e957b9d9e32a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12998,6 +12998,15 @@ F: Documentation/devicetree/bindings/mailbox/arm,mhuv2.yaml
> > F: drivers/mailbox/arm_mhuv2.c
> > F: include/linux/mailbox/arm_mhuv2_message.h
> >
> > +MAILBOX ARM MHUv3
> > +M: Sudeep Holla <sudeep.holla@arm.com>
> > +M: Cristian Marussi <cristian.marussi@arm.com>
> > +L: linux-kernel@vger.kernel.org
> > +L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
> > +S: Maintained
> > +F: Documentation/devicetree/bindings/mailbox/arm,mhuv3.yaml
> > +F: drivers/mailbox/arm_mhuv3.c
> > +
> > MAN-PAGES: MANUAL PAGES FOR LINUX -- Sections 2, 3, 4, 5, and 7
> > M: Alejandro Colomar <alx@kernel.org>
> > L: linux-man@vger.kernel.org
> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> > index 42940108a187..d20cdae65cfe 100644
> > --- a/drivers/mailbox/Kconfig
> > +++ b/drivers/mailbox/Kconfig
> > @@ -23,6 +23,17 @@ config ARM_MHU_V2
> > Say Y here if you want to build the ARM MHUv2 controller driver,
> > which provides unidirectional mailboxes between processing elements.
> >
> > +config ARM_MHU_V3
> > + tristate "ARM MHUv3 Mailbox"
> > + depends on ARM64 || COMPILE_TEST
> > + help
> > + Say Y here if you want to build the ARM MHUv3 controller driver,
> > + which provides unidirectional mailboxes between processing elements.
> > +
> > + ARM MHUv3 controllers can implement a varying number of extensions
> > + that provides different means of transports: supported extensions
> > + will be discovered and possibly managed at probe-time.
> > +
> > config IMX_MBOX
> > tristate "i.MX Mailbox"
> > depends on ARCH_MXC || COMPILE_TEST
> > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > index 18793e6caa2f..5cf2f54debaf 100644
> > --- a/drivers/mailbox/Makefile
> > +++ b/drivers/mailbox/Makefile
> > @@ -9,6 +9,8 @@ obj-$(CONFIG_ARM_MHU) += arm_mhu.o arm_mhu_db.o
> >
> > obj-$(CONFIG_ARM_MHU_V2) += arm_mhuv2.o
> >
> > +obj-$(CONFIG_ARM_MHU_V3) += arm_mhuv3.o
> > +
> > obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o
> >
> > obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX) += armada-37xx-rwtm-mailbox.o
> > 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 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ARM Message Handling Unit Version 3 (MHUv3) driver.
> > + *
> > + * Copyright (C) 2024 ARM Ltd.
> > + *
> > + * Based on ARM MHUv2 driver.
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mailbox_controller.h>
> > +#include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/types.h>
> > +
> > +/* ====== MHUv3 Registers ====== */
> > +
> > +/* Maximum number of Doorbell channel windows */
> > +#define MHUV3_DBCW_MAX 128
> > +/* Number of DBCH combined interrupt status registers */
> > +#define MHUV3_DBCH_CMB_INT_ST_REG_CNT 4
> > +#define MHUV3_INVALID_DOORBELL 0xFFFFFFFFUL
> > +
> > +/* Number of FFCH combined interrupt status registers */
> > +#define MHUV3_FFCH_CMB_INT_ST_REG_CNT 2
> > +
> > +#define MHUV3_STAT_BYTES (sizeof(u32))
> >
> Simply 4 please.
>
Ok.
> > +#define MHUV3_STAT_BITS (MHUV3_STAT_BYTES * __CHAR_BIT__)
> >
> just 32.
>
Ok.
> > +
> > +/* Not a typo ... */
> > +#define MHUV3_MAJOR_VERSION 2
> > +
> > +enum {
> > + MHUV3_MBOX_CELL_TYPE,
> > + MHUV3_MBOX_CELL_CHWN,
> > + MHUV3_MBOX_CELL_PARAM,
> > + MHUV3_MBOX_CELLS
> > +};
> > +
> > +/* CTRL_Page */
> > +
> > +struct blk_id {
> > + u32 blk_id : 4;
>
> Please avoid name clashes.
>
I'll fix.
> > + u32 pad : 28;
> > +} __packed;
> > +
> > +struct feat_spt0 {
> > + u32 dbe_spt : 4;
> > + u32 fe_spt : 4;
> > + u32 fce_spt : 4;
> > + u32 tze_spt : 4;
> > + u32 rme_spt : 4;
> > + u32 rase_spt : 4;
> > + u32 pad: 8;
> > +} __packed;
> > +
> > +struct feat_spt1 {
> > + u32 auto_op_spt : 4;
> > + u32 pad: 28;
> > +} __packed;
> > +
> > +struct dbch_cfg0 {
> > + u32 num_dbch : 8;
> > + u32 pad: 24;
> > +} __packed;
> > +
> > +struct ffch_cfg0 {
> > + u32 num_ffch : 8;
> > + u32 x8ba_spt : 1;
> > + u32 x16ba_spt : 1;
> > + u32 x32ba_spt : 1;
> > + u32 x64ba_spt : 1;
> > + u32 pad : 4;
> > + u32 ffch_depth : 10;
> > + u32 pad2 : 6;
> > +} __packed;
> > +
> > +struct fch_cfg0 {
> > + u32 num_fch : 10;
> > + /* MBX only registers */
> > + u32 fcgi_spt : 1;
> > + /* ------------------ */
> > + u32 num_fcg : 5;
> > + u32 num_fch_per_grp : 5;
> > + u32 fch_ws : 8;
> > + u32 pad : 3;
> > +} __packed;
> > +
> > +struct ctrl {
> > + u32 op_req : 1;
> > + u32 ch_op_mask : 1;
> > + u32 pad : 30;
> > +} __packed;
> > +
> > +struct fch_ctrl {
> > + u32 pad : 2;
> > + u32 int_en : 1;
> > + u32 pad2 : 29;
> > +} __packed;
> > +
> > +struct iidr {
> > + u32 implementer : 12;
> > + u32 revision : 4;
> > + u32 variant : 4;
> > + u32 product_id : 12;
> > +} __packed;
> > +
> > +struct aidr {
> > + u32 arch_minor_rev : 4;
> > + u32 arch_major_rev : 4;
> > + u32 pad : 24;
> > +} __packed;
> > +
> I am not sure about using bitfields on register values. I know v2
> driver also uses bitfields but this still is not very portable and is
> dependent on compiler behaviour. We may actually save some loc by not
> having unused fields if we use shifts and masks. Though I don't
> strongly feel either way.
>
Yes, indeed seemed a bit odd way of handling regs when I saw it in mhuv2,
BUT it seemed it had its advantages in terms of clarity of usage....did
not know about possible drawbacks, though. I'll re-think about the pros
and cons of this approach.
> > +struct ctrl_page {
> > + struct blk_id blk_id;
> > + u8 pad[0x10 - 0x4];
> > + struct feat_spt0 feat_spt0;
> > + struct feat_spt1 feat_spt1;
> > + u8 pad1[0x20 - 0x18];
> > + struct dbch_cfg0 dbch_cfg0;
> > + u8 pad2[0x30 - 0x24];
> > + struct ffch_cfg0 ffch_cfg0;
> > + u8 pad3[0x40 - 0x34];
> > + struct fch_cfg0 fch_cfg0;
> > + u8 pad4[0x100 - 0x44];
> > + struct ctrl ctrl;
> > + /* MBX only registers */
> > + u8 pad5[0x140 - 0x104];
> > + struct fch_ctrl fch_ctrl;
> > + u32 fcg_int_en;
> > + u8 pad6[0x400 - 0x148];
> > + /* ------------------ */
> Why the decoration ? Maybe comment on what different starts from here.
>
PBX and MBX Ctrl page are exactly the same, BUT for some registers banks
that does not exist in the PBX: this decoration is indeed the end, not
the start, of the MBX only regs that starts 5 lines above with the related
comment...was trying to avoid to use 2 different types for the basically
the same data...of course it works just because the PBX code refrains
from accessing the areas where only regs known to MBX lives.
> > + u32 dbch_int_st[MHUV3_DBCH_CMB_INT_ST_REG_CNT];
> > + u32 ffch_int_st[MHUV3_FFCH_CMB_INT_ST_REG_CNT];
> > + /* MBX only registers */
> > + u8 pad7[0x470 - 0x418];
> > + u32 fcg_int_st;
> > + u8 pad8[0x480 - 0x474];
> > + u32 fcg_grp_int_st[32];
> > + u8 pad9[0xFC8 - 0x500];
> > + /* ------------------ */
Same here.
> > + struct iidr iidr;
> > + struct aidr aidr;
> > + u32 imp_def_id[12];
> > +} __packed;
> > +
> > +/* DBCW_Page */
> > +
> > +struct xbcw_ctrl {
> > + u32 comb_en : 1;
> > + u32 pad : 31;
> > +} __packed;
> > +
> > +struct pdbcw_int {
> > + u32 tfr_ack : 1;
> > + u32 pad : 31;
> > +} __packed;
> > +
> > +struct pdbcw_page {
> > + u32 st;
> > + u8 pad[0xC - 0x4];
> > + u32 set;
> > + struct pdbcw_int int_st;
> > + struct pdbcw_int int_clr;
> > + struct pdbcw_int int_en;
> > + struct xbcw_ctrl ctrl;
> > +} __packed;
> > +
> > +struct mdbcw_page {
> > + u32 st;
> > + u32 st_msk;
> > + u32 clr;
> > + u8 pad[0x10 - 0xC];
> > + u32 msk_st;
> > + u32 msk_set;
> > + u32 msk_clr;
> > + struct xbcw_ctrl ctrl;
> > +} __packed;
> > +
> > +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];
> > + 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); \
> > + })
> > +
> > +/* ====== MHUv3 data structures ====== */
> > +
> > +enum mhuv3_frame {
> > + PBX_FRAME,
> > + MBX_FRAME
> > +};
> > +
> > +static char *mhuv3_str[] = {
> > + "PBX",
> > + "MBX"
> > +};
> > +
> > +enum mhuv3_extension_type {
> > + FIRST_EXT = 0,
> > + DBE_EXT = FIRST_EXT,
> > + FCE_EXT,
> > + FE_EXT,
> > + MAX_EXT
> > +};
> > +
> > +struct mhuv3;
> > +
> > +/**
> > + * struct mhuv3_protocol_ops - MHUv3 operations
> > + *
> > + * @rx_startup: Receiver startup callback.
> > + * @rx_shutdown: Receiver shutdown callback.
> > + * @read_data: Read available Sender in-band LE data (if any).
> > + * @rx_complete: Acknowledge data reception to the Sender. Any out-of-band data
> > + * has to have been already retrieved before calling this.
> > + * @tx_startup: Sender startup callback.
> > + * @tx_shutdown: Sender shutdown callback.
> > + * @last_tx_done: Report back to the Sender if the last transfer has completed.
> > + * @send_data: Send data to the receiver.
> > + *
> > + * Each supported transport protocol provides its own implementation of
> > + * these operations.
> > + */
> > +struct mhuv3_protocol_ops {
> > + int (*rx_startup)(struct mhuv3 *mhu, struct mbox_chan *chan);
> > + void (*rx_shutdown)(struct mhuv3 *mhu, struct mbox_chan *chan);
> > + void *(*read_data)(struct mhuv3 *mhu, struct mbox_chan *chan);
> > + void (*rx_complete)(struct mhuv3 *mhu, struct mbox_chan *chan);
> > + void (*tx_startup)(struct mhuv3 *mhu, struct mbox_chan *chan);
> > + void (*tx_shutdown)(struct mhuv3 *mhu, struct mbox_chan *chan);
> > + int (*last_tx_done)(struct mhuv3 *mhu, struct mbox_chan *chan);
> > + int (*send_data)(struct mhuv3 *mhu, struct mbox_chan *chan, void *arg);
> > +};
> > +
> > +/**
> > + * struct mhuv3_mbox_chan_priv - MHUv3 channel private information
> > + *
> > + * @ch_idx: Channel window index associated to this mailbox channel.
> > + * @doorbell: Doorbell bit number within the @ch_idx window.
> > + * Only relevant to Doorbell transport.
> > + * @ops: Transport protocol specific operations for this channel.
> > + *
> > + * Transport specific data attached to mmailbox channel priv data.
> > + */
> > +struct mhuv3_mbox_chan_priv {
> > + u32 ch_idx;
> > + u32 doorbell;
> > + const struct mhuv3_protocol_ops *ops;
> > +};
> > +
> > +/**
> > + * struct mhuv3_extension - MHUv3 extension descriptor
> > + *
> > + * @type: Type of extension
> > + * @max_chans: Max number of channels found for this extension.
> > + * @base_ch_idx: First channel number assigned to this extension, picked from
> > + * the set of all mailbox channels descriptors created.
> > + * @mbox_of_xlate: Extension specific helper to parse DT and lookup associated
> > + * channel from the related 'mboxes' property.
> > + * @combined_irq_setup: Extension specific helper to setup the combined irq.
> > + * @channels_init: Extension specific helper to initialize channels.
> > + * @chan_from_comb_irq_get: Extension specific helper to lookup which channel
> > + * triggered the combined irq.
> > + * @pending_db: Array of per-channel pending doorbells.
> > + * @pending_lock: Protect access to pending_db.
> > + */
> > +struct mhuv3_extension {
> > + enum mhuv3_extension_type type;
> > + unsigned int max_chans;
> > + unsigned int base_ch_idx;
> > + struct mbox_chan *(*mbox_of_xlate)(struct mhuv3 *mhu,
> > + unsigned int channel,
> > + unsigned int param);
> > + void (*combined_irq_setup)(struct mhuv3 *mhu);
> > + int (*channels_init)(struct mhuv3 *mhu);
> > + struct mbox_chan *(*chan_from_comb_irq_get)(struct mhuv3 *mhu);
> > + u32 pending_db[MHUV3_DBCW_MAX];
> > + /* Protect access to pending_db */
> > + spinlock_t pending_lock;
> > +};
> > +
> > +/**
> > + * struct mhuv3 - MHUv3 mailbox controller data
> > + *
> > + * @frame: Frame type: MBX_FRAME or PBX_FRAME.
> > + * @auto_op_full: Flag to indicate if the MHU supports AutoOp full mode.
> > + * @major: MHUv3 controller architectural major version.
> > + * @minor: MHUv3 controller architectural minor version.
> > + * @tot_chans: The total number of channnels discovered across all extensions.
> > + * @cmb_irq: Combined IRQ number if any found defined.
> > + * @ctrl: A reference to the MHUv3 control page for this block.
> > + * @pbx: Base address of the PBX register mapping region.
> > + * @mbx: Base address of the MBX register mapping region.
> > + * @ext: Array holding descriptors for any found implemented extension.
> > + * @mbox: Mailbox controller belonging to the MHU frame.
> > + */
> > +struct mhuv3 {
> > + enum mhuv3_frame frame;
> > + bool auto_op_full;
> > + unsigned int major;
> > + unsigned int minor;
> > + unsigned int tot_chans;
> >
> may be num_chans or chan_count ?
>
Ok.
>
> > + int cmb_irq;
> > + struct ctrl_page __iomem *ctrl;
> > + union {
> > + struct mhu3_pbx_frame_reg __iomem *pbx;
> > + struct mhu3_mbx_frame_reg __iomem *mbx;
> > + };
> > + struct mhuv3_extension *ext[MAX_EXT];
> > + struct mbox_controller mbox;
> > +};
> > +
> > +#define mhu_from_mbox(_mbox) container_of(_mbox, struct mhuv3, mbox)
> > +
> > +typedef int (*mhuv3_extension_initializer)(struct mhuv3 *mhu);
> > +
> > +/* =================== Doorbell transport protocol operations =============== */
> > +
> > +static void mhuv3_doorbell_tx_startup(struct mhuv3 *mhu, struct mbox_chan *chan)
> > +{
> > + struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
> > +
> > + /* Enable Transfer Acknowledgment events */
> > + writel_relaxed_bitfield(0x1, &mhu->pbx->dbcw[priv->ch_idx].int_en, tfr_ack);
> > +}
> > +
> > +static void mhuv3_doorbell_tx_shutdown(struct mhuv3 *mhu, struct mbox_chan *chan)
> > +{
> > + unsigned long flags;
> > + struct mhuv3_extension *e = mhu->ext[DBE_EXT];
> > + struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
> > +
> In order of decreasing line-lengths please everywhere.
>
Sure.
> > + /* Disable Channel Transfer Ack events */
> > + writel_relaxed_bitfield(0x0, &mhu->pbx->dbcw[priv->ch_idx].int_en, tfr_ack);
> > +
> > + /* Clear Channel Transfer Ack and pending doorbells */
> > + writel_relaxed_bitfield(0x1, &mhu->pbx->dbcw[priv->ch_idx].int_clr, tfr_ack);
> > + spin_lock_irqsave(&e->pending_lock, flags);
> > + e->pending_db[priv->ch_idx] = 0;
> > + spin_unlock_irqrestore(&e->pending_lock, flags);
> > +}
[snip]
> > +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);
>
> __ffs instead of __builtin_ctz please.
>
ok.
> > + if (channel >= e->max_chans) {
> > + dev_err(dev, "Invalid %s channel:%d\n",
> > + mhuv3_str[mhu->frame], channel);
> > + break;
> > + }
> > +
[snip]
> > +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)) {
> >
> 'continue' for error instead, to have fewer indented lines.
>
ok.
> > + 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)) {
> 'continue' for error instead, to have fewer indented lines.
>
ok.
Thanks,
Cristian
^ permalink raw reply
* [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document
From: Pavel Machek @ 2024-04-08 10:51 UTC (permalink / raw)
To: phone-devel, kernel list, fiona.klute, martijn, samuel,
heikki.krogerus, gregkh, linux-usb, robh+dt,
krzysztof.kozlowski+dt, devicetree, megi
[-- Attachment #1: Type: text/plain, Size: 4013 bytes --]
Add binding for anx7688 usb type-c bridge. I don't have a datasheet,
but I did best I could.
Signed-off-by: Pavel Machek <pavel@ucw.cz>
---
v2: implement review feedback
v3: fix single character pointed by robot
diff --git a/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml b/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml
new file mode 100644
index 000000000000..48b9ae936cb5
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml
@@ -0,0 +1,127 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/analogix,anx7688.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+# Pin names can be deduced from
+# https://files.pine64.org/doc/PinePhone/PinePhone%20v1.2b%20Released%20Schematic.pdf
+
+title: Analogix ANX7688 Type-C controller
+
+maintainers:
+ - Pavel Machek <pavel@ucw.cz>
+
+properties:
+ compatible:
+ enum:
+ - analogix,anx7688
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ reset-gpios:
+ maxItems: 1
+ description: GPIO controlling RESET_N (B7) pin.
+
+ enable-gpios:
+ maxItems: 1
+ description: GPIO controlling POWER_EN (D2) pin.
+
+ cabledet-gpios:
+ maxItems: 1
+ description: GPIO controlling CABLE_DET (C3) pin.
+
+ avdd10-supply:
+ description: 1.0V power supply going to AVDD10 (A4, ...) pins
+
+ dvdd10-supply:
+ description: 1.0V power supply going to DVDD10 (D6, ...) pins
+
+ avdd18-supply:
+ description: 1.8V power supply going to AVDD18 (E3, ...) pins
+
+ dvdd18-supply:
+ description: 1.8V power supply going to DVDD18 (G4, ...) pins
+
+ avdd33-supply:
+ description: 3.3V power supply going to AVDD33 (C4, ...) pins
+
+ i2c-supply: true
+ vconn-supply: true
+ hdmi-vt-supply: true
+ vbus-supply: true
+ vbus-in-supply: true
+
+ connector:
+ type: object
+ $ref: /schemas/connector/usb-connector.yaml
+
+ description:
+ Properties for usb c connector.
+
+ properties:
+ compatible:
+ const: usb-c-connector
+
+required:
+ - compatible
+ - reg
+ - connector
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/gpio/gpio.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ typec@2c {
+ compatible = "analogix,anx7688";
+ reg = <0x2c>;
+ interrupts = <8 IRQ_TYPE_EDGE_FALLING>;
+ interrupt-parent = <&gpio0>;
+
+ enable-gpios = <&pio 3 10 GPIO_ACTIVE_LOW>; /* PD10 */
+ reset-gpios = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */
+ cabledet-gpios = <&r_pio 0 8 GPIO_ACTIVE_HIGH>; /* PL8 */
+
+ avdd10-supply = <®_anx1v0>;
+ dvdd10-supply = <®_anx1v0>;
+ avdd18-supply = <®_ldo_io1>;
+ dvdd18-supply = <®_ldo_io1>;
+ avdd33-supply = <®_dcdc1>;
+ i2c-supply = <®_ldo_io0>;
+ vconn-supply = <®_vconn5v0>;
+ hdmi-vt-supply = <®_dldo1>;
+
+ vbus-supply = <®_usb_5v>;
+ vbus-in-supply = <&usb_power_supply>;
+
+ typec_con: connector {
+ compatible = "usb-c-connector";
+ power-role = "dual";
+ data-role = "dual";
+ try-power-role = "source";
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ port@0 {
+ reg = <0>;
+ typec_con_ep: endpoint {
+ remote-endpoint = <&usbotg_hs_ep>;
+ };
+ };
+ };
+ };
+ };
+ };
+...
--
People of Russia, stop Putin before his war on Ukraine escalates.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply related
* [PATCH] dt-bindings: pinctrl: mediatek: mt7622: add "gpio-ranges" property
From: Rafał Miłecki @ 2024-04-08 10:51 UTC (permalink / raw)
To: Sean Wang, Linus Walleij, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: Matthias Brugger, AngeloGioacchino Del Regno, Hsin-Yi Wang,
linux-mediatek, linux-gpio, devicetree, linux-arm-kernel,
Rafał Miłecki
From: Rafał Miłecki <rafal@milecki.pl>
Allow specifying pin to GPIO mapping. It can be find in in-Linux DTS
file for MT7622.
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
.../devicetree/bindings/pinctrl/mediatek,mt7622-pinctrl.yaml | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/pinctrl/mediatek,mt7622-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/mediatek,mt7622-pinctrl.yaml
index eaa65512161a..d211f2e403f2 100644
--- a/Documentation/devicetree/bindings/pinctrl/mediatek,mt7622-pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/mediatek,mt7622-pinctrl.yaml
@@ -36,6 +36,9 @@ properties:
the amount of cells must be specified as 2. See the below mentioned gpio
binding representation for description of particular cells.
+ gpio-ranges:
+ maxItems: 1
+
interrupt-controller: true
interrupts:
--
2.35.3
^ permalink raw reply related
* Re: [PATCH 0/5] Add parsing for Zimop ISA extension
From: Andrew Jones @ 2024-04-08 11:03 UTC (permalink / raw)
To: Clément Léger
Cc: Deepak Gupta, Jonathan Corbet, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Conor Dooley, Rob Herring, Krzysztof Kozlowski,
Anup Patel, Shuah Khan, Atish Patra, linux-doc, linux-riscv,
linux-kernel, devicetree, kvm, kvm-riscv, linux-kselftest
In-Reply-To: <ddc5555a-3ae8-42e5-a08a-ca5ceaf0bf28@rivosinc.com>
On Mon, Apr 08, 2024 at 10:01:12AM +0200, Clément Léger wrote:
>
>
> On 05/04/2024 19:33, Deepak Gupta wrote:
> > On Fri, Apr 5, 2024 at 8:26 AM Andrew Jones <ajones@ventanamicro.com> wrote:
> >>
> >> On Thu, Apr 04, 2024 at 12:32:46PM +0200, Clément Léger wrote:
> >>> The Zimop ISA extension was ratified recently. This series adds support
> >>> for parsing it from riscv,isa, hwprobe export and kvm support for
> >>> Guest/VM.
> >>
> >> I'm not sure we need this. Zimop by itself isn't useful, so I don't know
> >> if we need to advertise it at all. When an extension comes along that
> >> redefines some MOPs, then we'll advertise that extension, but the fact
> >> Zimop is used for that extension is really just an implementation detail.
> >
> > Only situation I see this can be useful is this:--
> >
> > An implementer, implemented Zimops in CPU solely for the purpose that they can
> > run mainline distro & packages on their hardware and don't want to leverage any
> > feature which are built on top of Zimop.
>
> Yes, the rationale was that some binaries using extensions that overload
> MOPs could still be run. With Zimop exposed, the loader could determine
> if the binary can be executed without potentially crashing. We could
> also let the program run anyway but the execution could potentially
> crash unexpectedly, which IMHO is not really good for the user
> experience nor for debugging. I already think that the segfaults which
> happens when executing binaries that need some missing extension are not
> so easy to debug, so better add more guards.
OK. It's only one more extension out of dozens, so I won't complain more,
but I was thinking that binaries that use particular extensions would
check for those particular extensions (step 2), rather than Zimop.
Thanks,
drew
>
> >
> > As an example zicfilp and zicfiss are dependent on zimops. glibc can
> > do following
> >
> > 1) check elf header if binary was compiled with zicfiss and zicfilp,
> > if yes goto step 2, else goto step 6.
> > 2) check if zicfiss/zicfilp is available in hw via hwprobe, if yes
> > goto step 5. else goto step 3
> > 3) check if zimop is available via hwprobe, if yes goto step 6, else goto step 4
>
> I think you meant step 5 rather than step 6.
>
> Clément
>
> > 4) This binary won't be able to run successfully on this platform,
> > issue exit syscall. <-- termination
> > 5) issue prctl to enable shadow stack and landing pad for current task
> > <-- enable feature
> > 6) let the binary run <-- let the binary run because no harm can be done
^ permalink raw reply
* Re: [PATCH 0/5] Add parsing for Zimop ISA extension
From: Andrew Jones @ 2024-04-08 11:04 UTC (permalink / raw)
To: Clément Léger
Cc: Jonathan Corbet, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Conor Dooley, Rob Herring, Krzysztof Kozlowski, Anup Patel,
Shuah Khan, Atish Patra, linux-doc, linux-riscv, linux-kernel,
devicetree, kvm, kvm-riscv, linux-kselftest
In-Reply-To: <20240404103254.1752834-1-cleger@rivosinc.com>
On Thu, Apr 04, 2024 at 12:32:46PM +0200, Clément Léger wrote:
> The Zimop ISA extension was ratified recently. This series adds support
> for parsing it from riscv,isa, hwprobe export and kvm support for
> Guest/VM.
>
> Clément Léger (5):
> dt-bindings: riscv: add Zimop ISA extension description
> riscv: add ISA extension parsing for Zimop
> riscv: hwprobe: export Zimop ISA extension
> RISC-V: KVM: Allow Zimop extension for Guest/VM
> KVM: riscv: selftests: Add Zimop extension to get-reg-list test
>
> Documentation/arch/riscv/hwprobe.rst | 4 ++++
> Documentation/devicetree/bindings/riscv/extensions.yaml | 5 +++++
> arch/riscv/include/asm/hwcap.h | 1 +
> arch/riscv/include/uapi/asm/hwprobe.h | 1 +
> arch/riscv/include/uapi/asm/kvm.h | 1 +
> arch/riscv/kernel/cpufeature.c | 1 +
> arch/riscv/kernel/sys_hwprobe.c | 1 +
> arch/riscv/kvm/vcpu_onereg.c | 2 ++
> tools/testing/selftests/kvm/riscv/get-reg-list.c | 4 ++++
> 9 files changed, 20 insertions(+)
>
> --
> 2.43.0
For the series,
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: mailbox: arm,mhuv3: Add bindings
From: Cristian Marussi @ 2024-04-08 11:09 UTC (permalink / raw)
To: Jassi Brar
Cc: linux-kernel, linux-arm-kernel, devicetree, sudeep.holla, robh+dt,
krzysztof.kozlowski+dt, conor+dt
In-Reply-To: <CABb+yY07OQQ8gAOZM2iV7nh_P5sQPWwNPODfwnJNZoPk8MNg2w@mail.gmail.com>
On Sun, Apr 07, 2024 at 06:38:52PM -0500, Jassi Brar wrote:
> On Thu, Apr 4, 2024 at 1:25 AM Cristian Marussi
> <cristian.marussi@arm.com> wrote:
> >
> > Add bindings for the ARM MHUv3 Mailbox controller.
> >
Hi,
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> > v2 -> v3
> > - fixed spurious tabs in dt_binding_check
> > v1 -> v2
> > - clarified extension descriptions around configurability and discoverability
> > - removed unused labels from the example
> > - using pattern properties to define interrupt-names
> > - bumped interrupt maxItems to 74 (allowing uo to 8 channels per extension)
> > ---
> > .../bindings/mailbox/arm,mhuv3.yaml | 217 ++++++++++++++++++
> > 1 file changed, 217 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/mailbox/arm,mhuv3.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhuv3.yaml b/Documentation/devicetree/bindings/mailbox/arm,mhuv3.yaml
> > new file mode 100644
> > index 000000000000..32a8bb711464
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/arm,mhuv3.yaml
> > @@ -0,0 +1,217 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mailbox/arm,mhuv3.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ARM MHUv3 Mailbox Controller
> > +
> > +maintainers:
> > + - Sudeep Holla <sudeep.holla@arm.com>
> > + - Cristian Marussi <cristian.marussi@arm.com>
> > +
> > +description: |
> > + The Arm Message Handling Unit (MHU) Version 3 is a mailbox controller that
> > + enables unidirectional communications with remote processors through various
> > + possible transport protocols.
> > + The controller can optionally support a varying number of extensions that, in
> > + turn, enable different kinds of transport to be used for communication.
> > + Number, type and characteristics of each supported extension can be discovered
> > + dynamically at runtime.
> > +
> > + Given the unidirectional nature of the controller, an MHUv3 mailbox controller
> > + is composed of a MHU Sender (MHUS) containing a PostBox (PBX) block and a MHU
> > + Receiver (MHUR) containing a MailBox (MBX) block, where
> > +
> > + PBX is used to
> > + - Configure the MHU
> > + - Send Transfers to the Receiver
> > + - Optionally receive acknowledgment of a Transfer from the Receiver
> > +
> > + MBX is used to
> > + - Configure the MHU
> > + - Receive Transfers from the Sender
> > + - Optionally acknowledge Transfers sent by the Sender
> > +
> > + Both PBX and MBX need to be present and defined in the DT description if you
> > + need to establish a bidirectional communication, since you will have to
> > + acquire two distinct unidirectional channels, one for each block.
> > +
> > + As a consequence both blocks needs to be represented separately and specified
> > + as distinct DT nodes in order to properly describe their resources.
> > +
> > + Note that, though, thanks to the runtime discoverability, there is no need to
> > + identify the type of blocks with distinct compatibles.
> > +
> > + Following are the MHUv3 possible extensions.
> > +
> > + - Doorbell Extension (DBE): DBE defines a type of channel called a Doorbell
> > + Channel (DBCH). DBCH enables a single bit Transfer to be sent from the
> > + Sender to Receiver. The Transfer indicates that an event has occurred.
> > + When DBE is implemented, the number of DBCHs that an implementation of the
> > + MHU can support is between 1 and 128, numbered starting from 0 in ascending
> > + order and discoverable at run-time.
> > + Each DBCH contains 32 individual fields, referred to as flags, each of which
> > + can be used independently. It is possible for the Sender to send multiple
> > + Transfers at once using a single DBCH, so long as each Transfer uses
> > + a different flag in the DBCH.
> > + Optionally, data may be transmitted through an out-of-band shared memory
> > + region, wherein the MHU Doorbell is used strictly as an interrupt generation
> > + mechanism, but this is out of the scope of these bindings.
> > +
> > + - FastChannel Extension (FCE): FCE defines a type of channel called a Fast
> > + Channel (FCH). FCH is intended for lower overhead communication between
> > + Sender and Receiver at the expense of determinism. An FCH allows the Sender
> > + to update the channel value at any time, regardless of whether the previous
> > + value has been seen by the Receiver. When the Receiver reads the channel's
> > + content it gets the last value written to the channel.
> > + FCH is considered lossy in nature, and means that the Sender has no way of
> > + knowing if, or when, the Receiver will act on the Transfer.
> > + FCHs are expected to behave as RAM which generates interrupts when writes
> > + occur to the locations within the RAM.
> > + When FCE is implemented, the number of FCHs that an implementation of the
> > + MHU can support is between 1-1024, if the FastChannel word-size is 32-bits,
> > + or between 1-512, when the FastChannel word-size is 64-bits.
> > + FCHs are numbered from 0 in ascending order.
> > + Note that the number of FCHs and the word-size are implementation defined,
> > + not configurable but discoverable at run-time.
> > + Optionally, data may be transmitted through an out-of-band shared memory
> > + region, wherein the MHU FastChannel is used as an interrupt generation
> > + mechanism which carries also a pointer to such out-of-band data, but this
> > + is out of the scope of these bindings.
> > +
> > + - FIFO Extension (FE): FE defines a Channel type called a FIFO Channel (FFCH).
> > + FFCH allows a Sender to send
> > + - Multiple Transfers to the Receiver without having to wait for the
> > + previous Transfer to be acknowledged by the Receiver, as long as the
> > + FIFO has room for the Transfer.
> > + - Transfers which require the Receiver to provide acknowledgment.
> > + - Transfers which have in-band payload.
> > + In all cases, the data is guaranteed to be observed by the Receiver in the
> > + same order which the Sender sent it.
> > + When FE is implemented, the number of FFCHs that an implementation of the
> > + MHU can support is between 1 and 64, numbered starting from 0 in ascending
> > + order. The number of FFCHs, their depth (same for all implemented FFCHs) and
> > + the access-granularity are implementation defined, not configurable but
> > + discoverable at run-time.
> > + Optionally, additional data may be transmitted through an out-of-band shared
> > + memory region, wherein the MHU FIFO is used to transmit, in order, a small
> > + part of the payload (like a header) and a reference to the shared memory
> > + area holding the remaining, bigger, chunk of the payload, but this is out of
> > + the scope of these bindings.
> > +
> > +properties:
> > + compatible:
> > + const: arm,mhuv3
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + minItems: 1
> > + maxItems: 74
> > +
> > + interrupt-names:
> > + description: |
> > + The MHUv3 controller generates a number of events some of which are used
> > + to generate interrupts; as a consequence it can expose a varying number of
> > + optional PBX/MBX interrupts, representing the events generated during the
> > + operation of the various transport protocols associated with different
> > + extensions. All interrupts of the MHU are level-sensitive.
> > + Some of these optional interrupts are defined per-channel, where the
> > + number of channels effectively available is implementation defined and
> > + run-time discoverable.
> > + In the following names are enumerated using patterns, with per-channel
> > + interrupts implicitly capped at the maximum channels allowed by the
> > + specification for each extension type.
> > + For the sake of simplicity maxItems is anyway capped to a most plausible
> > + number, assuming way less channels would be implemented than actually
> > + possible.
> > +
> > + The only mandatory interrupts on the MHU are:
> > + - combined
> > + - mbx-fch-xfer-<N> but only if mbx-fcgrp-xfer-<N> is not implemented.
> > +
> > + minItems: 1
> > + maxItems: 74
> > + items:
> > + oneOf:
> > + - const: combined
> > + description: PBX/MBX Combined interrupt
> > + - const: combined-ffch
> > + description: PBX/MBX FIFO Combined interrupt
> > + - pattern: '^ffch-low-tide-[0-9]+$'
> > + description: PBX/MBX FIFO Channel <N> Low Tide interrupt
> > + - pattern: '^ffch-high-tide-[0-9]+$'
> > + description: PBX/MBX FIFO Channel <N> High Tide interrupt
> > + - pattern: '^ffch-flush-[0-9]+$'
> > + description: PBX/MBX FIFO Channel <N> Flush interrupt
> > + - pattern: '^mbx-dbch-xfer-[0-9]+$'
> > + description: MBX Doorbell Channel <N> Transfer interrupt
> > + - pattern: '^mbx-fch-xfer-[0-9]+$'
> > + description: MBX FastChannel <N> Transfer interrupt
> > + - pattern: '^mbx-fchgrp-xfer-[0-9]+$'
> > + description: MBX FastChannel <N> Group Transfer interrupt
> > + - pattern: '^mbx-ffch-xfer-[0-9]+$'
> > + description: MBX FIFO Channel <N> Transfer interrupt
> > + - pattern: '^pbx-dbch-xfer-ack-[0-9]+$'
> > + description: PBX Doorbell Channel <N> Transfer Ack interrupt
> > + - pattern: '^pbx-ffch-xfer-ack-[0-9]+$'
> > + description: PBX FIFO Channel <N> Transfer Ack interrupt
> > +
> Can we have optional subnodes (with different properties as required)
> for each extension type ?
>
Not sure if I have understood properly you request, but the type of extensions
present in a PBX/MBX block can be discovered at runtime together with their
characteristics (like the number of channels) so the specific DT properties
can be searched (or NOT) based on the features discovered at run-time:
are you asking for subnodes as a means of adding clarity to what can be
defined in a block depending on what it is (PBX vs MBX) and what
extensions it has ?
if that is the reason ... the convoluted names like pbx/mbx-dbch- etc...
was my attempt at thhat :D ... to give a hint at what you can define in a PBX
vs MBX block and what is related to each extensions...
Note that, though, since the PBX/MBX blocks are discoverable at runtime as such,
they are not identified as such in the DT (same compatible) so I would not have
anywayy the capability to check in the DT which is which (PBX/MBX) and if the
properties are appropriate or not, nor I could know which extensions are really
implemented, so such subnodes would ony be a way of grouping props in the DT
without adding any compile time check capability nor adding any improvement to
the runtime DT parsing proces...
...BUT, of course, I could be wrong and missing a something here, so I
am happy to corrected on the subnodes utility...
>
> > + '#mbox-cells':
> > + description: |
> > + The first argument in the consumers 'mboxes' property represents the
> > + extension type, the second is for the channel number while the third
> > + depends on extension type.
> > +
> > + Extension type for DBE is 0 and the third parameter represents the
> > + doorbell flag number to use.
> > + Extension type for FCE is 1, third parameter unused.
> > + Extension type for FE is 2, third parameter unused.
> > +
> > + mboxes = <&mhu 0 0 5>; // DBE, Doorbell Channel Window 0, doorbell flag 5.
> > + mboxes = <&mhu 0 1 7>; // DBE, Doorbell Channel Window 1, doorbell flag 7.
> > + mboxes = <&mhu 1 0 0>; // FCE, FastChannel Window 0.
> > + mboxes = <&mhu 1 3 0>; // FCE, FastChannel Window 3.
> > + mboxes = <&mhu 2 1 0>; // FE, FIFO Channel Window 1.
> > + mboxes = <&mhu 2 7 0>; // FE, FIFO Channel Window 7.
> >
> Please define the extension types, instead of 0, 1 and 2.
>
I'll do.
Thanks for the review.
Cristian
^ permalink raw reply
* Re: [PATCH v5 1/2] media: dt-bindings: nxp,imx8-jpeg:: Add clocks entries
From: Fabio Estevam @ 2024-04-08 11:14 UTC (permalink / raw)
To: Mirela Rabulea
Cc: shawnguo, robh+dt, krzysztof.kozlowski+dt, festevam,
alexander.stein, Frank.li, ming.qian, conor+dt, devicetree,
linux-arm-kernel, s.hauer, kernel, mchehab, hverkuil, linux-media,
imx, linux-kernel
In-Reply-To: <20240408075806.1194297-1-mirela.rabulea@nxp.com>
Hi Mirela,
On Mon, Apr 8, 2024 at 4:58 AM Mirela Rabulea <mirela.rabulea@nxp.com> wrote:
>
> From: Fabio Estevam <festevam@gmail.com>
>
> The JPEG decoder/encoder present in iMX8QXP and iMX8QM SoCs need
> the PER and IPG clocks to be functional, so add the clock entries.
>
> This also fixes the following schema warning:
>
> imx8qm-apalis-eval.dtb: jpegdec@58400000: 'assigned-clock-rates', 'assigned-clocks', 'clock-names', 'clocks' do not match any of the regexes: 'pinctrl-[0-9]+'
> from schema $id: http://devicetree.org/schemas/media/nxp,imx8-jpeg.yaml#
>
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com>
Thanks for submitting this series.
In the Subject, you added an extra : "..nxp,imx8-jpeg:: Add". Please
remove the extra :.
My From and Signed-off-by tags e-mails do not match.
Please use the email from the Signed-off-by in the From field as well.
The same applies to patch 2/2.
Thanks
^ permalink raw reply
* Re: [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document
From: Krzysztof Kozlowski @ 2024-04-08 11:17 UTC (permalink / raw)
To: Pavel Machek, phone-devel, kernel list, fiona.klute, martijn,
samuel, heikki.krogerus, gregkh, linux-usb, robh+dt,
krzysztof.kozlowski+dt, devicetree, megi
In-Reply-To: <ZhPMHdt6r/4D99Zg@duo.ucw.cz>
On 08/04/2024 12:51, Pavel Machek wrote:
> Add binding for anx7688 usb type-c bridge. I don't have a datasheet,
> but I did best I could.
>
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
...
> + cabledet-gpios:
> + maxItems: 1
> + description: GPIO controlling CABLE_DET (C3) pin.
> +
> + avdd10-supply:
> + description: 1.0V power supply going to AVDD10 (A4, ...) pins
> +
> + dvdd10-supply:
> + description: 1.0V power supply going to DVDD10 (D6, ...) pins
> +
> + avdd18-supply:
> + description: 1.8V power supply going to AVDD18 (E3, ...) pins
> +
> + dvdd18-supply:
> + description: 1.8V power supply going to DVDD18 (G4, ...) pins
> +
> + avdd33-supply:
> + description: 3.3V power supply going to AVDD33 (C4, ...) pins
> +
> + i2c-supply: true
> + vconn-supply: true
There are no such supplies like i2c and vconn on the schematics.
I think this represents some other part of component which was added
here only for convenience.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 0/5] Add parsing for Zimop ISA extension
From: Clément Léger @ 2024-04-08 11:19 UTC (permalink / raw)
To: Andrew Jones
Cc: Deepak Gupta, Jonathan Corbet, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Conor Dooley, Rob Herring, Krzysztof Kozlowski,
Anup Patel, Shuah Khan, Atish Patra, linux-doc, linux-riscv,
linux-kernel, devicetree, kvm, kvm-riscv, linux-kselftest
In-Reply-To: <20240408-6c93f3f50b55234f3825ca33@orel>
On 08/04/2024 13:03, Andrew Jones wrote:
> On Mon, Apr 08, 2024 at 10:01:12AM +0200, Clément Léger wrote:
>>
>>
>> On 05/04/2024 19:33, Deepak Gupta wrote:
>>> On Fri, Apr 5, 2024 at 8:26 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>>>>
>>>> On Thu, Apr 04, 2024 at 12:32:46PM +0200, Clément Léger wrote:
>>>>> The Zimop ISA extension was ratified recently. This series adds support
>>>>> for parsing it from riscv,isa, hwprobe export and kvm support for
>>>>> Guest/VM.
>>>>
>>>> I'm not sure we need this. Zimop by itself isn't useful, so I don't know
>>>> if we need to advertise it at all. When an extension comes along that
>>>> redefines some MOPs, then we'll advertise that extension, but the fact
>>>> Zimop is used for that extension is really just an implementation detail.
>>>
>>> Only situation I see this can be useful is this:--
>>>
>>> An implementer, implemented Zimops in CPU solely for the purpose that they can
>>> run mainline distro & packages on their hardware and don't want to leverage any
>>> feature which are built on top of Zimop.
>>
>> Yes, the rationale was that some binaries using extensions that overload
>> MOPs could still be run. With Zimop exposed, the loader could determine
>> if the binary can be executed without potentially crashing. We could
>> also let the program run anyway but the execution could potentially
>> crash unexpectedly, which IMHO is not really good for the user
>> experience nor for debugging. I already think that the segfaults which
>> happens when executing binaries that need some missing extension are not
>> so easy to debug, so better add more guards.
>
> OK. It's only one more extension out of dozens, so I won't complain more,
No worries, your point *is* valid since I'm not sure yet that the loader
will actually do that one day.
BTW, are you aware of any effort to make the elf dynamic loader
"smarter" and actually check for needed extensions to be present rather
than blindly running the elf and potentially catching SIGILL ?
Thanks,
Clément
> but I was thinking that binaries that use particular extensions would
> check for those particular extensions (step 2), rather than Zimop.
>
> Thanks,
> drew
>
>>
>>>
>>> As an example zicfilp and zicfiss are dependent on zimops. glibc can
>>> do following
>>>
>>> 1) check elf header if binary was compiled with zicfiss and zicfilp,
>>> if yes goto step 2, else goto step 6.
>>> 2) check if zicfiss/zicfilp is available in hw via hwprobe, if yes
>>> goto step 5. else goto step 3
>>> 3) check if zimop is available via hwprobe, if yes goto step 6, else goto step 4
>>
>> I think you meant step 5 rather than step 6.
>>
>> Clément
>>
>>> 4) This binary won't be able to run successfully on this platform,
>>> issue exit syscall. <-- termination
>>> 5) issue prctl to enable shadow stack and landing pad for current task
>>> <-- enable feature
>>> 6) let the binary run <-- let the binary run because no harm can be done
^ permalink raw reply
* Re: [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document
From: Pavel Machek @ 2024-04-08 11:21 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: phone-devel, kernel list, fiona.klute, martijn, samuel,
heikki.krogerus, gregkh, linux-usb, robh+dt,
krzysztof.kozlowski+dt, devicetree, megi
In-Reply-To: <ab9affc8-de68-4ec9-bdfc-02131191bc3a@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 1199 bytes --]
Hi!
> > Add binding for anx7688 usb type-c bridge. I don't have a datasheet,
> > but I did best I could.
> >
> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
>
> ...
>
> > + cabledet-gpios:
> > + maxItems: 1
> > + description: GPIO controlling CABLE_DET (C3) pin.
> > +
> > + avdd10-supply:
> > + description: 1.0V power supply going to AVDD10 (A4, ...) pins
> > +
> > + dvdd10-supply:
> > + description: 1.0V power supply going to DVDD10 (D6, ...) pins
> > +
> > + avdd18-supply:
> > + description: 1.8V power supply going to AVDD18 (E3, ...) pins
> > +
> > + dvdd18-supply:
> > + description: 1.8V power supply going to DVDD18 (G4, ...) pins
> > +
> > + avdd33-supply:
> > + description: 3.3V power supply going to AVDD33 (C4, ...) pins
> > +
> > + i2c-supply: true
> > + vconn-supply: true
>
> There are no such supplies like i2c and vconn on the schematics.
>
> I think this represents some other part of component which was added
> here only for convenience.
Can you give me pointer to documentation you are looking at?
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document
From: Krzysztof Kozlowski @ 2024-04-08 11:24 UTC (permalink / raw)
To: Pavel Machek
Cc: phone-devel, kernel list, fiona.klute, martijn, samuel,
heikki.krogerus, gregkh, linux-usb, robh+dt,
krzysztof.kozlowski+dt, devicetree, megi
In-Reply-To: <ZhPTTxI4oTF3pgrk@duo.ucw.cz>
On 08/04/2024 13:21, Pavel Machek wrote:
> Hi!
>
>>> Add binding for anx7688 usb type-c bridge. I don't have a datasheet,
>>> but I did best I could.
>>>
>>> Signed-off-by: Pavel Machek <pavel@ucw.cz>
>>
>> ...
>>
>>> + cabledet-gpios:
>>> + maxItems: 1
>>> + description: GPIO controlling CABLE_DET (C3) pin.
>>> +
>>> + avdd10-supply:
>>> + description: 1.0V power supply going to AVDD10 (A4, ...) pins
>>> +
>>> + dvdd10-supply:
>>> + description: 1.0V power supply going to DVDD10 (D6, ...) pins
>>> +
>>> + avdd18-supply:
>>> + description: 1.8V power supply going to AVDD18 (E3, ...) pins
>>> +
>>> + dvdd18-supply:
>>> + description: 1.8V power supply going to DVDD18 (G4, ...) pins
>>> +
>>> + avdd33-supply:
>>> + description: 3.3V power supply going to AVDD33 (C4, ...) pins
>>> +
>>> + i2c-supply: true
>>> + vconn-supply: true
>>
>> There are no such supplies like i2c and vconn on the schematics.
>>
>> I think this represents some other part of component which was added
>> here only for convenience.
>
> Can you give me pointer to documentation you are looking at?
The schematics you linked in the document at the beginning. Page 13. Do
you see these pins there? I saw only VCONN1_EN, but that's not a supply.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH] dt-bindings: pinctrl: mediatek: mt7622: add "gpio-ranges" property
From: Krzysztof Kozlowski @ 2024-04-08 11:25 UTC (permalink / raw)
To: Rafał Miłecki, Sean Wang, Linus Walleij, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: Matthias Brugger, AngeloGioacchino Del Regno, Hsin-Yi Wang,
linux-mediatek, linux-gpio, devicetree, linux-arm-kernel,
Rafał Miłecki
In-Reply-To: <20240408105128.30586-1-zajec5@gmail.com>
On 08/04/2024 12:51, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> Allow specifying pin to GPIO mapping. It can be find in in-Linux DTS
> file for MT7622.
>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
Acked-by: Krzysztof Kozlowski <krzk@kernel.org>
Best regards,
Krzysztof
^ 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