* [PATCH v2 0/2] Meson: R/W support for pages used by boot ROM
From: Arseniy Krasnov @ 2024-04-08 8:59 UTC (permalink / raw)
To: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
Kevin Hilman, Jerome Brunet, Martin Blumenstingl
Cc: linux-mtd, devicetree, linux-arm-kernel, linux-amlogic,
linux-kernel, oxffffaa, kernel, Arseniy Krasnov
Amlogic's boot ROM code needs that some pages on NAND must be written
in special "short" ECC mode with scrambling enabled. Such pages:
1) Contain some metadata about hardware.
2) Located with some interval starting from 0 offset, until some
specified offset. Interval and second offset are set in the
device tree.
This patchset adds R/W support for such pages. To enable it we can setup
it in dts:
nand-is-boot-medium;
amlogic,boot-page-last = <1024>;
amlogic,boot-page-step = <128>;
It means that each 128th page in range 0 to 1024 pages will be accessed
in special mode ("short" ECC + scrambling). In practice this feature is
needed when we want to update first block of NAND - driver will enable
required mode by itself using value from device tree.
Changelog:
v1 -> v2:
* Rename 'meson,boot-page-XXX' -> 'amlogic,boot-page-XXX'.
* Add words that 'amlogic,boot-page-step' is measured in pages.
* Remove words that 'amlogic,boot-page-XXX' depends on 'nand-is-boot-medium'.
* Make both 'amlogic,boot-page-XXX' depend on each other also, in
addition to 'nand-is-boot-medium' dependency.
Arseniy Krasnov (2):
dt-bindings: mtd: amlogic,meson-nand: support fields for boot ROM code
mtd: rawnand: meson: support R/W mode for boot ROM
.../bindings/mtd/amlogic,meson-nand.yaml | 14 +++
drivers/mtd/nand/raw/meson_nand.c | 88 +++++++++++++------
2 files changed, 73 insertions(+), 29 deletions(-)
--
2.35.0
^ permalink raw reply
* [PATCH v2 2/2] mtd: rawnand: meson: support R/W mode for boot ROM
From: Arseniy Krasnov @ 2024-04-08 8:59 UTC (permalink / raw)
To: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
Kevin Hilman, Jerome Brunet, Martin Blumenstingl
Cc: linux-mtd, devicetree, linux-arm-kernel, linux-amlogic,
linux-kernel, oxffffaa, kernel, Arseniy Krasnov
In-Reply-To: <20240408085931.456337-1-avkrasnov@salutedevices.com>
Boot ROM code on Meson requires that some pages on NAND must be written
in special mode: "short" ECC mode where each block is 384 bytes and
scrambling mode is on. Such pages located with the specified interval
within specified offset. Both interval and offset are located in the
device tree and used by driver if 'nand-is-boot-medium' is set for
NAND chip.
Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com>
---
drivers/mtd/nand/raw/meson_nand.c | 88 +++++++++++++++++++++----------
1 file changed, 59 insertions(+), 29 deletions(-)
diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
index 00ce0e5bb970..b68aac24af16 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -35,6 +35,7 @@
#define NFC_CMD_RB BIT(20)
#define NFC_CMD_SCRAMBLER_ENABLE BIT(19)
#define NFC_CMD_SCRAMBLER_DISABLE 0
+#define NFC_CMD_SHORTMODE_ENABLE 1
#define NFC_CMD_SHORTMODE_DISABLE 0
#define NFC_CMD_RB_INT BIT(14)
#define NFC_CMD_RB_INT_NO_PIN ((0xb << 10) | BIT(18) | BIT(16))
@@ -78,6 +79,8 @@
#define DMA_DIR(dir) ((dir) ? NFC_CMD_N2M : NFC_CMD_M2N)
#define DMA_ADDR_ALIGN 8
+#define NFC_SHORT_MODE_ECC_SZ 384
+
#define ECC_CHECK_RETURN_FF (-1)
#define NAND_CE0 (0xe << 10)
@@ -125,6 +128,8 @@ struct meson_nfc_nand_chip {
u32 twb;
u32 tadl;
u32 tbers_max;
+ u32 boot_page_last;
+ u32 boot_page_step;
u32 bch_mode;
u8 *data_buf;
@@ -298,28 +303,49 @@ static void meson_nfc_cmd_seed(struct meson_nfc *nfc, u32 seed)
nfc->reg_base + NFC_REG_CMD);
}
-static void meson_nfc_cmd_access(struct nand_chip *nand, int raw, bool dir,
- int scrambler)
+static int meson_nfc_page_is_boot(struct nand_chip *nand, int page)
+{
+ const struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
+
+ return (nand->options & NAND_IS_BOOT_MEDIUM) &&
+ !(page % meson_chip->boot_page_step) &&
+ (page < meson_chip->boot_page_last);
+}
+
+static void meson_nfc_cmd_access(struct nand_chip *nand, bool raw, bool dir, int page)
{
+ const struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
struct mtd_info *mtd = nand_to_mtd(nand);
struct meson_nfc *nfc = nand_get_controller_data(mtd_to_nand(mtd));
- struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
- u32 bch = meson_chip->bch_mode, cmd;
int len = mtd->writesize, pagesize, pages;
+ int scrambler;
+ u32 cmd;
- pagesize = nand->ecc.size;
+ if (nand->options & NAND_NEED_SCRAMBLING)
+ scrambler = NFC_CMD_SCRAMBLER_ENABLE;
+ else
+ scrambler = NFC_CMD_SCRAMBLER_DISABLE;
if (raw) {
len = mtd->writesize + mtd->oobsize;
cmd = len | scrambler | DMA_DIR(dir);
- writel(cmd, nfc->reg_base + NFC_REG_CMD);
- return;
- }
+ } else if (meson_nfc_page_is_boot(nand, page)) {
+ pagesize = NFC_SHORT_MODE_ECC_SZ >> 3;
+ pages = mtd->writesize / 512;
+
+ scrambler = NFC_CMD_SCRAMBLER_ENABLE;
+ cmd = CMDRWGEN(DMA_DIR(dir), scrambler, NFC_ECC_BCH8_1K,
+ NFC_CMD_SHORTMODE_ENABLE, pagesize, pages);
+ } else {
+ pagesize = nand->ecc.size >> 3;
+ pages = len / nand->ecc.size;
- pages = len / nand->ecc.size;
+ cmd = CMDRWGEN(DMA_DIR(dir), scrambler, meson_chip->bch_mode,
+ NFC_CMD_SHORTMODE_DISABLE, pagesize, pages);
+ }
- cmd = CMDRWGEN(DMA_DIR(dir), scrambler, bch,
- NFC_CMD_SHORTMODE_DISABLE, pagesize, pages);
+ if (scrambler == NFC_CMD_SCRAMBLER_ENABLE)
+ meson_nfc_cmd_seed(nfc, page);
writel(cmd, nfc->reg_base + NFC_REG_CMD);
}
@@ -743,15 +769,7 @@ static int meson_nfc_write_page_sub(struct nand_chip *nand,
if (ret)
return ret;
- if (nand->options & NAND_NEED_SCRAMBLING) {
- meson_nfc_cmd_seed(nfc, page);
- meson_nfc_cmd_access(nand, raw, DIRWRITE,
- NFC_CMD_SCRAMBLER_ENABLE);
- } else {
- meson_nfc_cmd_access(nand, raw, DIRWRITE,
- NFC_CMD_SCRAMBLER_DISABLE);
- }
-
+ meson_nfc_cmd_access(nand, raw, DIRWRITE, page);
cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_PAGEPROG;
writel(cmd, nfc->reg_base + NFC_REG_CMD);
meson_nfc_queue_rb(nand, PSEC_TO_MSEC(sdr->tPROG_max), false);
@@ -829,15 +847,7 @@ static int meson_nfc_read_page_sub(struct nand_chip *nand,
if (ret)
return ret;
- if (nand->options & NAND_NEED_SCRAMBLING) {
- meson_nfc_cmd_seed(nfc, page);
- meson_nfc_cmd_access(nand, raw, DIRREAD,
- NFC_CMD_SCRAMBLER_ENABLE);
- } else {
- meson_nfc_cmd_access(nand, raw, DIRREAD,
- NFC_CMD_SCRAMBLER_DISABLE);
- }
-
+ meson_nfc_cmd_access(nand, raw, DIRREAD, page);
ret = meson_nfc_wait_dma_finish(nfc);
meson_nfc_check_ecc_pages_valid(nfc, nand, raw);
@@ -1436,6 +1446,26 @@ meson_nfc_nand_chip_init(struct device *dev,
if (ret)
return ret;
+ if (nand->options & NAND_IS_BOOT_MEDIUM) {
+ ret = of_property_read_u32(np, "amlogic,boot-page-last",
+ &meson_chip->boot_page_last);
+ if (ret) {
+ dev_err(dev, "could not retrieve 'amlogic,boot-page-last' property: %d",
+ ret);
+ nand_cleanup(nand);
+ return ret;
+ }
+
+ ret = of_property_read_u32(np, "amlogic,boot-page-step",
+ &meson_chip->boot_page_step);
+ if (ret) {
+ dev_err(dev, "could not retrieve 'amlogic,boot-page-step' property: %d",
+ ret);
+ nand_cleanup(nand);
+ return ret;
+ }
+ }
+
ret = mtd_device_register(mtd, NULL, 0);
if (ret) {
dev_err(dev, "failed to register MTD device: %d\n", ret);
--
2.35.0
^ permalink raw reply related
* [PATCH v2 1/2] dt-bindings: mtd: amlogic,meson-nand: support fields for boot ROM code
From: Arseniy Krasnov @ 2024-04-08 8:59 UTC (permalink / raw)
To: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
Kevin Hilman, Jerome Brunet, Martin Blumenstingl
Cc: linux-mtd, devicetree, linux-arm-kernel, linux-amlogic,
linux-kernel, oxffffaa, kernel, Arseniy Krasnov
In-Reply-To: <20240408085931.456337-1-avkrasnov@salutedevices.com>
Boot ROM code on Meson requires that some pages on NAND must be written
in special mode: "short" ECC mode where each block is 384 bytes and
scrambling mode is on. Such pages located with the specified interval
within specified offset. Both interval and offset are located in the
device tree and used by driver if 'nand-is-boot-medium' is set for
NAND chip.
Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com>
---
.../bindings/mtd/amlogic,meson-nand.yaml | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml
index 57b6957c8415..80ba5003ca70 100644
--- a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml
+++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml
@@ -64,11 +64,25 @@ patternProperties:
items:
maximum: 0
+ amlogic,boot-page-last:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ The NFC driver needs this information to select ECC
+ algorithms supported by the boot ROM.
+
+ amlogic,boot-page-step:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ The NFC driver needs this information to select ECC
+ algorithms supported by the boot ROM (in pages).
+
unevaluatedProperties: false
dependencies:
nand-ecc-strength: [nand-ecc-step-size]
nand-ecc-step-size: [nand-ecc-strength]
+ amlogic,boot-page-last: [nand-is-boot-medium, amlogic,boot-page-step]
+ amlogic,boot-page-step: [nand-is-boot-medium, amlogic,boot-page-last]
required:
--
2.35.0
^ permalink raw reply related
* Re: [RESEND v3 0/2] Add support for QCM6490 and QCS6490
From: Srinivas Kandagatla @ 2024-04-08 9:16 UTC (permalink / raw)
To: Mohammad Rafi Shaik, Banajit Goswami, Liam Girdwood, Mark Brown,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jaroslav Kysela,
Takashi Iwai
Cc: linux-arm-msm, alsa-devel, linux-sound, devicetree, linux-kernel,
quic_rohkumar
In-Reply-To: <20240408042331.403103-1-quic_mohs@quicinc.com>
On 08/04/2024 05:23, Mohammad Rafi Shaik wrote:
> This patchset adds support for sound card on Qualcomm QCM6490 IDP and
> QCS6490 RB3Gen2 boards.
>
> Changes since v2:
> - Modify qcm6490 compatible name as qcm6490-idp. Suggested by Dmitry
>
> Changes since v1:
> - Use existing sc8280xp machine driver instead of separate driver.
> - Modify qcs6490 compatible name as qcs6490-rb3gen2.
>
> Mohammad Rafi Shaik (2):
> ASoC: dt-bindings: qcom,sm8250: Add QCM6490 snd QCS6490 sound card
> ASoC: qcom: sc8280xp: Add support for QCM6490 and QCS6490
>
LGTM,
Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
--srini
> Documentation/devicetree/bindings/sound/qcom,sm8250.yaml | 2 ++
> sound/soc/qcom/sc8280xp.c | 2 ++
> 2 files changed, 4 insertions(+)
>
^ permalink raw reply
* Re: [PATCH v12 2/7] clk: meson: add vclk driver
From: neil.armstrong @ 2024-04-08 9:19 UTC (permalink / raw)
To: Jerome Brunet
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Martin Blumenstingl, Kevin Hilman, Michael Turquette,
Stephen Boyd, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Daniel Vetter, Jagan Teki, Nicolas Belin,
devicetree, linux-kernel, linux-amlogic, linux-clk,
linux-arm-kernel, dri-devel
In-Reply-To: <1ja5m8p4n4.fsf@starbuckisacylon.baylibre.com>
On 05/04/2024 09:00, Jerome Brunet wrote:
>
> On Thu 04 Apr 2024 at 18:59, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
>> On 04/04/2024 10:13, Jerome Brunet wrote:
>>> On Wed 03 Apr 2024 at 09:46, Neil Armstrong <neil.armstrong@linaro.org>
>>> wrote:
>>>
>>>> The VCLK and VCLK_DIV clocks have supplementary bits.
>>>>
>>>> The VCLK gate has a "SOFT RESET" bit to toggle after the whole
>>>> VCLK sub-tree rate has been set, this is implemented in
>>>> the gate enable callback.
>>>>
>>>> The VCLK_DIV clocks as enable and reset bits used to disable
>>>> and reset the divider, associated with CLK_SET_RATE_GATE it ensures
>>>> the rate is set while the divider is disabled and in reset mode.
>>>>
>>>> The VCLK_DIV enable bit isn't implemented as a gate since it's part
>>>> of the divider logic and vendor does this exact sequence to ensure
>>>> the divider is correctly set.
>>> The checkpatch warning is still there. Is it a choice or a mistake ?
>>> Documentation says "GPL v2" exists for historic reason which seems to
>>> hint "GPL" is preferred, and I suppose this is why checkpatch warns for
>>> it.
>>
>> Well I didn't see this warning, this is what I fixed:
>>
>> $ scripts/checkpatch.pl --strict drivers/clk/meson/vclk.c
>> CHECK: Alignment should match open parenthesis
>> #63: FILE: drivers/clk/meson/vclk.c:63:
>> +static unsigned long meson_vclk_div_recalc_rate(struct clk_hw *hw,
>> + unsigned long prate)
>>
>> CHECK: Alignment should match open parenthesis
>> #73: FILE: drivers/clk/meson/vclk.c:73:
>> +static int meson_vclk_div_determine_rate(struct clk_hw *hw,
>> + struct clk_rate_request *req)
>>
>> CHECK: Alignment should match open parenthesis
>> #83: FILE: drivers/clk/meson/vclk.c:83:
>> +static int meson_vclk_div_set_rate(struct clk_hw *hw, unsigned long rate,
>> + unsigned long parent_rate)
>>
>
> I would not ask a respin solely for this. It's nice to fix it but I was
> mostly after the warning TBH.
>
>> <snip>
>>
>> It seems that checking a commit triggers an extra check....
>>
>> $ scripts/checkpatch.pl --strict -G 1bac9f6aa3c3
>> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
>> #58:
>> new file mode 100644
>>
>> <snip>
>>
>> WARNING: Prefer "GPL" over "GPL v2" - see commit bf7fbeeae6db ("module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2" bogosity")
>> #203: FILE: drivers/clk/meson/vclk.c:141:
>> +MODULE_LICENSE("GPL v2");
>
> Hum, I'm running checkpatch against the mail itself, not the commit. I
> still get the warning
Patch or commit seems to trigger more tests than a file directly, anyway I sent a follow-up patch:
https://lore.kernel.org/all/20240408-amlogic-v6-9-upstream-fix-clk-module-license-v1-1-366ddc0f3db9@linaro.org/
Thanks,
Neil
>
>>
>> <snip>
>>
>> Neil
>>
>>>
>>>>
>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>> ---
>>>> drivers/clk/meson/Kconfig | 4 ++
>>>> drivers/clk/meson/Makefile | 1 +
>>>> drivers/clk/meson/vclk.c | 141 +++++++++++++++++++++++++++++++++++++++++++++
>>>> drivers/clk/meson/vclk.h | 51 ++++++++++++++++
>>>> 4 files changed, 197 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>>>> index 29ffd14d267b..8a9823789fa3 100644
>>>> --- a/drivers/clk/meson/Kconfig
>>>> +++ b/drivers/clk/meson/Kconfig
>>>> @@ -30,6 +30,10 @@ config COMMON_CLK_MESON_VID_PLL_DIV
>>>> tristate
>>>> select COMMON_CLK_MESON_REGMAP
>>>> +config COMMON_CLK_MESON_VCLK
>>>> + tristate
>>>> + select COMMON_CLK_MESON_REGMAP
>>>> +
>>>> config COMMON_CLK_MESON_CLKC_UTILS
>>>> tristate
>>>> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>>>> index 9ee4b954c896..9ba43fe7a07a 100644
>>>> --- a/drivers/clk/meson/Makefile
>>>> +++ b/drivers/clk/meson/Makefile
>>>> @@ -12,6 +12,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_PLL) += clk-pll.o
>>>> obj-$(CONFIG_COMMON_CLK_MESON_REGMAP) += clk-regmap.o
>>>> obj-$(CONFIG_COMMON_CLK_MESON_SCLK_DIV) += sclk-div.o
>>>> obj-$(CONFIG_COMMON_CLK_MESON_VID_PLL_DIV) += vid-pll-div.o
>>>> +obj-$(CONFIG_COMMON_CLK_MESON_VCLK) += vclk.o
>>>> # Amlogic Clock controllers
>>>> diff --git a/drivers/clk/meson/vclk.c b/drivers/clk/meson/vclk.c
>>>> new file mode 100644
>>>> index 000000000000..45dc216941ea
>>>> --- /dev/null
>>>> +++ b/drivers/clk/meson/vclk.c
>>>> @@ -0,0 +1,141 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Copyright (c) 2024 Neil Armstrong <neil.armstrong@linaro.org>
>>>> + */
>>>> +
>>>> +#include <linux/module.h>
>>>> +#include "vclk.h"
>>>> +
>>>> +/* The VCLK gate has a supplementary reset bit to pulse after ungating */
>>>> +
>>>> +static inline struct meson_vclk_gate_data *
>>>> +clk_get_meson_vclk_gate_data(struct clk_regmap *clk)
>>>> +{
>>>> + return (struct meson_vclk_gate_data *)clk->data;
>>>> +}
>>>> +
>>>> +static int meson_vclk_gate_enable(struct clk_hw *hw)
>>>> +{
>>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>>> + struct meson_vclk_gate_data *vclk = clk_get_meson_vclk_gate_data(clk);
>>>> +
>>>> + meson_parm_write(clk->map, &vclk->enable, 1);
>>>> +
>>>> + /* Do a reset pulse */
>>>> + meson_parm_write(clk->map, &vclk->reset, 1);
>>>> + meson_parm_write(clk->map, &vclk->reset, 0);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void meson_vclk_gate_disable(struct clk_hw *hw)
>>>> +{
>>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>>> + struct meson_vclk_gate_data *vclk = clk_get_meson_vclk_gate_data(clk);
>>>> +
>>>> + meson_parm_write(clk->map, &vclk->enable, 0);
>>>> +}
>>>> +
>>>> +static int meson_vclk_gate_is_enabled(struct clk_hw *hw)
>>>> +{
>>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>>> + struct meson_vclk_gate_data *vclk = clk_get_meson_vclk_gate_data(clk);
>>>> +
>>>> + return meson_parm_read(clk->map, &vclk->enable);
>>>> +}
>>>> +
>>>> +const struct clk_ops meson_vclk_gate_ops = {
>>>> + .enable = meson_vclk_gate_enable,
>>>> + .disable = meson_vclk_gate_disable,
>>>> + .is_enabled = meson_vclk_gate_is_enabled,
>>>> +};
>>>> +EXPORT_SYMBOL_GPL(meson_vclk_gate_ops);
>>>> +
>>>> +/* The VCLK Divider has supplementary reset & enable bits */
>>>> +
>>>> +static inline struct meson_vclk_div_data *
>>>> +clk_get_meson_vclk_div_data(struct clk_regmap *clk)
>>>> +{
>>>> + return (struct meson_vclk_div_data *)clk->data;
>>>> +}
>>>> +
>>>> +static unsigned long meson_vclk_div_recalc_rate(struct clk_hw *hw,
>>>> + unsigned long prate)
>>>> +{
>>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>>> + struct meson_vclk_div_data *vclk = clk_get_meson_vclk_div_data(clk);
>>>> +
>>>> + return divider_recalc_rate(hw, prate, meson_parm_read(clk->map, &vclk->div),
>>>> + vclk->table, vclk->flags, vclk->div.width);
>>>> +}
>>>> +
>>>> +static int meson_vclk_div_determine_rate(struct clk_hw *hw,
>>>> + struct clk_rate_request *req)
>>>> +{
>>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>>> + struct meson_vclk_div_data *vclk = clk_get_meson_vclk_div_data(clk);
>>>> +
>>>> + return divider_determine_rate(hw, req, vclk->table, vclk->div.width,
>>>> + vclk->flags);
>>>> +}
>>>> +
>>>> +static int meson_vclk_div_set_rate(struct clk_hw *hw, unsigned long rate,
>>>> + unsigned long parent_rate)
>>>> +{
>>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>>> + struct meson_vclk_div_data *vclk = clk_get_meson_vclk_div_data(clk);
>>>> + int ret;
>>>> +
>>>> + ret = divider_get_val(rate, parent_rate, vclk->table, vclk->div.width,
>>>> + vclk->flags);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + meson_parm_write(clk->map, &vclk->div, ret);
>>>> +
>>>> + return 0;
>>>> +};
>>>> +
>>>> +static int meson_vclk_div_enable(struct clk_hw *hw)
>>>> +{
>>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>>> + struct meson_vclk_div_data *vclk = clk_get_meson_vclk_div_data(clk);
>>>> +
>>>> + /* Unreset the divider when ungating */
>>>> + meson_parm_write(clk->map, &vclk->reset, 0);
>>>> + meson_parm_write(clk->map, &vclk->enable, 1);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void meson_vclk_div_disable(struct clk_hw *hw)
>>>> +{
>>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>>> + struct meson_vclk_div_data *vclk = clk_get_meson_vclk_div_data(clk);
>>>> +
>>>> + /* Reset the divider when gating */
>>>> + meson_parm_write(clk->map, &vclk->enable, 0);
>>>> + meson_parm_write(clk->map, &vclk->reset, 1);
>>>> +}
>>>> +
>>>> +static int meson_vclk_div_is_enabled(struct clk_hw *hw)
>>>> +{
>>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>>> + struct meson_vclk_div_data *vclk = clk_get_meson_vclk_div_data(clk);
>>>> +
>>>> + return meson_parm_read(clk->map, &vclk->enable);
>>>> +}
>>>> +
>>>> +const struct clk_ops meson_vclk_div_ops = {
>>>> + .recalc_rate = meson_vclk_div_recalc_rate,
>>>> + .determine_rate = meson_vclk_div_determine_rate,
>>>> + .set_rate = meson_vclk_div_set_rate,
>>>> + .enable = meson_vclk_div_enable,
>>>> + .disable = meson_vclk_div_disable,
>>>> + .is_enabled = meson_vclk_div_is_enabled,
>>>> +};
>>>> +EXPORT_SYMBOL_GPL(meson_vclk_div_ops);
>>>> +
>>>> +MODULE_DESCRIPTION("Amlogic vclk clock driver");
>>>> +MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
>>>> +MODULE_LICENSE("GPL v2");
>>>> diff --git a/drivers/clk/meson/vclk.h b/drivers/clk/meson/vclk.h
>>>> new file mode 100644
>>>> index 000000000000..20b0b181db09
>>>> --- /dev/null
>>>> +++ b/drivers/clk/meson/vclk.h
>>>> @@ -0,0 +1,51 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/*
>>>> + * Copyright (c) 2024 Neil Armstrong <neil.armstrong@linaro.org>
>>>> + */
>>>> +
>>>> +#ifndef __VCLK_H
>>>> +#define __VCLK_H
>>>> +
>>>> +#include "clk-regmap.h"
>>>> +#include "parm.h"
>>>> +
>>>> +/**
>>>> + * struct meson_vclk_gate_data - vclk_gate regmap backed specific data
>>>> + *
>>>> + * @enable: vclk enable field
>>>> + * @reset: vclk reset field
>>>> + * @flags: hardware-specific flags
>>>> + *
>>>> + * Flags:
>>>> + * Same as clk_gate except CLK_GATE_HIWORD_MASK which is ignored
>>>> + */
>>>> +struct meson_vclk_gate_data {
>>>> + struct parm enable;
>>>> + struct parm reset;
>>>> + u8 flags;
>>>> +};
>>>> +
>>>> +extern const struct clk_ops meson_vclk_gate_ops;
>>>> +
>>>> +/**
>>>> + * struct meson_vclk_div_data - vclk_div regmap back specific data
>>>> + *
>>>> + * @div: divider field
>>>> + * @enable: vclk divider enable field
>>>> + * @reset: vclk divider reset field
>>>> + * @table: array of value/divider pairs, last entry should have div = 0
>>>> + *
>>>> + * Flags:
>>>> + * Same as clk_divider except CLK_DIVIDER_HIWORD_MASK which is ignored
>>>> + */
>>>> +struct meson_vclk_div_data {
>>>> + struct parm div;
>>>> + struct parm enable;
>>>> + struct parm reset;
>>>> + const struct clk_div_table *table;
>>>> + u8 flags;
>>>> +};
>>>> +
>>>> +extern const struct clk_ops meson_vclk_div_ops;
>>>> +
>>>> +#endif /* __VCLK_H */
>>>
>
>
^ permalink raw reply
* Re: [PATCH v2] dt-bindings: display: bridge: it6505: Add #sound-dai-cells
From: Doug Anderson @ 2024-04-08 9:11 UTC (permalink / raw)
To: Rob Herring, Chen-Yu Tsai
Cc: Laurent Pinchart, Krzysztof Kozlowski, dri-devel, Maxime Ripard,
Neil Armstrong, Daniel Vetter, Robert Foss, Rob Herring,
Conor Dooley, Thomas Zimmermann, David Airlie, linux-kernel,
devicetree, Jonas Karlman, Andrzej Hajda, Jernej Skrabec,
Maarten Lankhorst
In-Reply-To: <171156080808.3681700.13600868771478432605.robh@kernel.org>
Hi,
On Wed, Mar 27, 2024 at 10:33 AM Rob Herring <robh@kernel.org> wrote:
>
>
> On Wed, 27 Mar 2024 16:52:48 +0800, Chen-Yu Tsai wrote:
> > The ITE IT6505 display bridge can take one I2S input and transmit it
> > over the DisplayPort link.
> >
> > Add #sound-dai-cells (= 0) to the binding for it.
> >
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> > Changes since v1 [1]:
> > - Reference /schemas/sound/dai-common.yaml
> > - Change "additionalProperties: false" to "unevaluatedProperties: false"
> >
> > The driver side changes [2] are still being worked on.
> >
> > [1] https://lore.kernel.org/dri-devel/20240126073511.2708574-1-wenst@chromium.org/
> > [2] https://lore.kernel.org/linux-arm-kernel/20230730180803.22570-4-jiaxin.yu@mediatek.com/
> > ---
> > .../devicetree/bindings/display/bridge/ite,it6505.yaml | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
>
> Reviewed-by: Rob Herring <robh@kernel.org>
Pushed to drm-misc-next:
325af1bef5b9 dt-bindings: display: bridge: it6505: Add #sound-dai-cells
^ permalink raw reply
* [PATCH] arm64: dts: imx8mp-msc-sm2s: Add i2c{1,6} sda-/scl-gpios
From: Ian Ray @ 2024-04-08 9:24 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam
Cc: Ian Ray, devicetree, imx, linux-arm-kernel, linux-kernel
Add i2c{1,6} sda-/scl-gpios with the corresponding pinmux entries.
Signed-off-by: Ian Ray <ian.ray@gehealthcare.com>
---
.../boot/dts/freescale/imx8mp-msc-sm2s.dtsi | 22 +++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/freescale/imx8mp-msc-sm2s.dtsi b/arch/arm64/boot/dts/freescale/imx8mp-msc-sm2s.dtsi
index 61c2a63efc6d..7e4327084d26 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp-msc-sm2s.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp-msc-sm2s.dtsi
@@ -200,8 +200,11 @@ ethphy1: ethernet-phy@1 {
};
&i2c1 {
- pinctrl-names = "default";
+ pinctrl-names = "default", "gpio";
pinctrl-0 = <&pinctrl_i2c1>;
+ pinctrl-1 = <&pinctrl_i2c1_gpio>;
+ scl-gpios = <&gpio5 14 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+ sda-gpios = <&gpio5 15 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
clock-frequency = <400000>;
status = "okay";
@@ -241,8 +244,11 @@ &i2c5 {
};
&i2c6 {
- pinctrl-names = "default";
+ pinctrl-names = "default", "gpio";
pinctrl-0 = <&pinctrl_i2c6>;
+ pinctrl-1 = <&pinctrl_i2c6_gpio>;
+ scl-gpios = <&gpio3 19 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+ sda-gpios = <&gpio3 20 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
clock-frequency = <400000>;
status = "okay";
@@ -606,6 +612,12 @@ pinctrl_i2c1: i2c1grp {
<MX8MP_IOMUXC_I2C1_SDA__I2C1_SDA 0x400001c3>;
};
+ pinctrl_i2c1_gpio: i2c1gpiogrp {
+ fsl,pins =
+ <MX8MP_IOMUXC_I2C1_SCL__GPIO5_IO14 0x400001c3>,
+ <MX8MP_IOMUXC_I2C1_SDA__GPIO5_IO15 0x400001c3>;
+ };
+
pinctrl_i2c2: i2c2grp {
fsl,pins =
<MX8MP_IOMUXC_I2C2_SCL__I2C2_SCL 0x400001c3>,
@@ -636,6 +648,12 @@ pinctrl_i2c6: i2c6grp {
<MX8MP_IOMUXC_SAI5_RXC__I2C6_SDA 0x400001c3>;
};
+ pinctrl_i2c6_gpio: i2c5gpiogrp {
+ fsl,pins =
+ <MX8MP_IOMUXC_SAI5_RXFS__GPIO3_IO19 0x400001c3>,
+ <MX8MP_IOMUXC_SAI5_RXC__GPIO3_IO20 0x400001c3>;
+ };
+
pinctrl_lcd0_backlight: lcd0-backlightgrp {
fsl,pins =
<MX8MP_IOMUXC_GPIO1_IO05__GPIO1_IO05 0x41>;
--
2.39.2
^ permalink raw reply related
* [PATCH v3 0/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
From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
This series is for adding support of high-end specs ICM-42688-P chip.
Changelog:
* v2: change order of chip definitions and order of patches
* v3: add patch description and resend without email IP header issue
Jean-Baptiste Maneyrol (2):
dt-bindings: iio: imu: add icm42688 inside inv_icm42600
iio: imu: inv_icm42600: add support of ICM-42688-P
.../devicetree/bindings/iio/imu/invensense,icm42600.yaml | 1 +
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 +++
5 files changed, 14 insertions(+)
--
2.34.1
^ permalink raw reply
* Re: [PATCH v9 6/6] PCI: qcom: Add OPP support to scale performance state of power domain
From: Manivannan Sadhasivam @ 2024-04-08 9:45 UTC (permalink / raw)
To: Krishna Chaitanya Chundru
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: <6e9b4379-5849-73cd-4d89-5e809b4c71a4@quicinc.com>
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.
- Mani
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply
* Re: [PATCH v3 5/5] arm64: dts: ti: k3-am62*: Add PHY2 region to USB wrapper node
From: Francesco Dolcini @ 2024-04-08 9:52 UTC (permalink / raw)
To: Roger Quadros
Cc: nm, vigneshr, afd, kristo, robh+dt, krzysztof.kozlowski+dt,
conor+dt, srk, r-gunasekaran, b-liu, linux-arm-kernel, devicetree,
linux-kernel
In-Reply-To: <20240201120332.4811-6-rogerq@kernel.org>
Hello Roger,
On Thu, Feb 01, 2024 at 02:03:32PM +0200, Roger Quadros wrote:
> Add PHY2 register space to USB wrapper node. This is required
> to deal with Errata i2409.
>
> Signed-off-by: Roger Quadros <rogerq@kernel.org>
What's the status/plan for this? v6.9-rc misses it and therefore we have
this error in the logs (and of course, we miss the workaround).
[ 0.583305] dwc3-am62 f910000.dwc3-usb: invalid resource (null)
[ 0.589304] dwc3-am62 f910000.dwc3-usb: can't map PHY IOMEM resource. Won't apply i2409 fix.
Apart for the error message, the change here seems required for the
hardware to properly function (IOW IMHO it should be back-ported to stable).
Francesco
^ permalink raw reply
* 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
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