* [PATCH 1/2] arm64: allwinner: h6: add I2C nodes
From: Bhushan Shah @ 2019-08-11 9:05 UTC (permalink / raw)
To: Icenowy Zheng, Maxime Ripard, Chen-Yu Tsai, Rob Herring,
Mark Rutland, linux-arm-kernel, devicetree, linux-kernel
Cc: Bhushan Shah
In-Reply-To: <20190811090503.32396-1-bshah@kde.org>
Add device-tree nodes for i2c0 to i2c2, and also add relevant pinctrl
nodes.
Suggested-by: Icenowy Zheng <icenowy@aosc.io>
Signed-off-by: Bhushan Shah <bshah@kde.org>
---
arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 54 ++++++++++++++++++++
1 file changed, 54 insertions(+)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
index bcecca17d61d..1d9ad3ec0b65 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -329,6 +329,21 @@
function = "hdmi";
};
+ i2c0_pins: i2c0-pins {
+ pins = "PD25", "PD26";
+ function = "i2c0";
+ };
+
+ i2c1_pins: i2c1-pins {
+ pins = "PH5", "PH6";
+ function = "i2c1";
+ };
+
+ i2c2_pins: i2c2-pins {
+ pins = "PD23", "PD24";
+ function = "i2c2";
+ };
+
mmc0_pins: mmc0-pins {
pins = "PF0", "PF1", "PF2", "PF3",
"PF4", "PF5";
@@ -464,6 +479,45 @@
status = "disabled";
};
+ i2c0: i2c@5002000 {
+ compatible = "allwinner,sun6i-a31-i2c";
+ reg = <0x05002000 0x400>;
+ interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&ccu CLK_BUS_I2C0>;
+ resets = <&ccu RST_BUS_I2C0>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&i2c0_pins>;
+ status = "disabled";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ };
+
+ i2c1: i2c@5002400 {
+ compatible = "allwinner,sun6i-a31-i2c";
+ reg = <0x05002400 0x400>;
+ interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&ccu CLK_BUS_I2C1>;
+ resets = <&ccu RST_BUS_I2C1>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&i2c1_pins>;
+ status = "disabled";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ };
+
+ i2c2: i2c@5002800 {
+ compatible = "allwinner,sun6i-a31-i2c";
+ reg = <0x05002800 0x400>;
+ interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&ccu CLK_BUS_I2C2>;
+ resets = <&ccu RST_BUS_I2C2>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&i2c2_pins>;
+ status = "disabled";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ };
+
emac: ethernet@5020000 {
compatible = "allwinner,sun50i-h6-emac",
"allwinner,sun50i-a64-emac";
--
2.17.1
^ permalink raw reply related
* [PATCH 0/2] Enable the I2C nodes for Allwinner H6 CPU
From: Bhushan Shah @ 2019-08-11 9:05 UTC (permalink / raw)
To: Icenowy Zheng, Maxime Ripard, Chen-Yu Tsai, Rob Herring,
Mark Rutland, linux-arm-kernel, devicetree, linux-kernel
Cc: Bhushan Shah
This patch series adds device-tree nodes for i2c nodes in the H6 dtsi,
and enables it for the Pine H64.
Bhushan Shah (2):
arm64: allwinner: h6: add I2C nodes
arm64: allwinner: h6: enable i2c0 in PineH64
.../boot/dts/allwinner/sun50i-h6-pine-h64.dts | 4 ++
arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 54 +++++++++++++++++++
2 files changed, 58 insertions(+)
--
2.17.1
^ permalink raw reply
* [PATCH v2 3/9] dt-bindings: display: panel: Add bindings for NEC NL8048HL11 panel
From: Laurent Pinchart @ 2019-08-10 23:10 UTC (permalink / raw)
To: dri-devel; +Cc: devicetree, Thierry Reding, Sam Ravnborg, Rob Herring
In-Reply-To: <20190810231048.1921-1-laurent.pinchart@ideasonboard.com>
The NEC NL8048HL11 is a 10.4cm WVGA (800x480) panel with a 24-bit RGB
parallel data interface and an SPI control interface.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:
- Convert to YAML
---
.../display/panel/nec,nl8048hl11.yaml | 49 +++++++++++++++++++
1 file changed, 49 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/panel/nec,nl8048hl11.yaml
diff --git a/Documentation/devicetree/bindings/display/panel/nec,nl8048hl11.yaml b/Documentation/devicetree/bindings/display/panel/nec,nl8048hl11.yaml
new file mode 100644
index 000000000000..cc3d40975828
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/nec,nl8048hl11.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/nec,nl8048hl11.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NEC NL8048HL11 4.1" WVGA TFT LCD panel
+
+description:
+ The NEC NL8048HL11 is a 4.1" WVGA TFT LCD panel with a 24-bit RGB parallel
+ data interface and an SPI control interface.
+
+maintainers:
+ - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+
+allOf:
+ - $ref: panel-common.yaml#
+
+properties:
+ compatible:
+ const: nec,nl8048hl11
+
+ label: true
+ reset-gpios: true
+ port: true
+
+required:
+ - compatible
+ - reset-gpios
+ - port
+
+additionalProperties: false
+
+examples:
+ - |
+ lcd_panel: panel {
+ compatible = "nec,nl8048hl11";
+ spi-max-frequency = <10000000>;
+
+ reset-gpios = <&gpio7 7 GPIO_ACTIVE_LOW>;
+
+ port {
+ lcd_in: endpoint {
+ remote-endpoint = <&dpi_out>;
+ };
+ };
+ };
+
+...
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH v2 2/9] dt-bindings: Add legacy 'toppoly' vendor prefix
From: Laurent Pinchart @ 2019-08-10 23:10 UTC (permalink / raw)
To: dri-devel; +Cc: devicetree, Thierry Reding, Sam Ravnborg, Rob Herring
In-Reply-To: <20190810231048.1921-1-laurent.pinchart@ideasonboard.com>
The 'toppoly' vendor prefix is in use and refers to TPO, whose DT vendor
prefix is already defined as 'tpo'. Add 'toppoly' as an alternative and
document it as legacy.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:
- Mark the prefix as deprecated
---
Documentation/devicetree/bindings/vendor-prefixes.yaml | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 5efddbff2430..29dcc6f8a64a 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -935,6 +935,9 @@ patternProperties:
description: Tecon Microprocessor Technologies, LLC.
"^topeet,.*":
description: Topeet
+ "^toppoly,.*":
+ description: TPO (deprecated, use tpo)
+ deprecated: true
"^toradex,.*":
description: Toradex AG
"^toshiba,.*":
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH v2 0/9] DRM panel drivers for omapdrm
From: Laurent Pinchart @ 2019-08-10 23:10 UTC (permalink / raw)
To: dri-devel; +Cc: devicetree, Thierry Reding, Sam Ravnborg, Rob Herring
Hello everybody,
These 9 patches have initially been posted as part of the larger "[PATCH
00/60] drm/omap: Replace custom display drivers with drm_bridge and
drm_panel" series, hence the v2 in the subject prefix.
I'm posting this second version separately per Sam's request as the rest
of the original series is expected to take more time to process through
review.
There's nothing very special here. The first three patches add DT vendor
prefixes and DT bindings. Since v1 patch 3/9 has been converted from
text to YAML, and as I'm not very familiar with YAML DT bindings, I'm
eagerly waiting for reviews.
The last six patches add new panel drivers. The code originates from the
corresponding omapdrm-specific panel drivers, which explains why only
one new DT binding is needed as most of them are already present.
Please see individual patches for changelogs. Sam, I believe I've taken
all your comments into account, I hope none fell through the cracks.
The patches are based on top of drm-misc-next and can be found at
git://linuxtv.org/pinchartl/media.git omapdrm/panels
Laurent Pinchart (9):
dt-bindings: Add vendor prefix for LG Display
dt-bindings: Add legacy 'toppoly' vendor prefix
dt-bindings: display: panel: Add bindings for NEC NL8048HL11 panel
drm/panel: Add driver for the LG Philips LB035Q02 panel
drm/panel: Add driver for the NEC NL8048HL11 panel
drm/panel: Add driver for the Sharp LS037V7DW01 panel
drm/panel: Add driver for the Sony ACX565AKM panel
drm/panel: Add driver for the Toppoly TD028TTEC1 panel
drm/panel: Add driver for the Toppoly TD043MTEA1 panel
.../display/panel/nec,nl8048hl11.yaml | 49 ++
.../devicetree/bindings/vendor-prefixes.yaml | 5 +
drivers/gpu/drm/panel/Kconfig | 46 ++
drivers/gpu/drm/panel/Makefile | 6 +
drivers/gpu/drm/panel/panel-lg-lb035q02.c | 237 ++++++
drivers/gpu/drm/panel/panel-nec-nl8048hl11.c | 247 +++++++
.../gpu/drm/panel/panel-sharp-ls037v7dw01.c | 226 ++++++
drivers/gpu/drm/panel/panel-sony-acx565akm.c | 693 ++++++++++++++++++
drivers/gpu/drm/panel/panel-tpo-td028ttec1.c | 381 ++++++++++
drivers/gpu/drm/panel/panel-tpo-td043mtea1.c | 508 +++++++++++++
10 files changed, 2398 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/panel/nec,nl8048hl11.yaml
create mode 100644 drivers/gpu/drm/panel/panel-lg-lb035q02.c
create mode 100644 drivers/gpu/drm/panel/panel-nec-nl8048hl11.c
create mode 100644 drivers/gpu/drm/panel/panel-sharp-ls037v7dw01.c
create mode 100644 drivers/gpu/drm/panel/panel-sony-acx565akm.c
create mode 100644 drivers/gpu/drm/panel/panel-tpo-td028ttec1.c
create mode 100644 drivers/gpu/drm/panel/panel-tpo-td043mtea1.c
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH] of: Fix of_empty_ranges_quirk()
From: Rob Herring @ 2019-08-10 23:06 UTC (permalink / raw)
To: Marek Vasut
Cc: devicetree, Marek Vasut, Frank Rowand,
open list:MEDIA DRIVERS FOR RENESAS - FCP
In-Reply-To: <10818888-6476-f4b1-1a2e-e10c3159327f@gmail.com>
On Sat, Aug 10, 2019 at 7:39 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>
> On 8/10/19 12:34 AM, Rob Herring wrote:
> > On Fri, Aug 9, 2019 at 11:33 AM <marek.vasut@gmail.com> wrote:
> >>
> >> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> >>
> >> The of_empty_ranges_quirk() returns a mix of boolean and signed integer
> >> types, which cannot work well.
> >
> > It never returns a negative. The negative is used as an uninitialized
> > flag. Note quirk_state is static.
>
> It's still mixing boolean and signed int types though, which isn't right.
I'm really only interested in touching this code if it is too remove
it. But some reason people still run 1990s Macs.
> >> Replace that with boolean only and fix
> >> usage logic in of_translate_one() -- the check should trigger when the
> >> ranges are NULL and the quirk is applicable on the hardware.
> >>
> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >> Cc: Rob Herring <robh+dt@kernel.org>
> >> Cc: Frank Rowand <frowand.list@gmail.com>
> >> Cc: linux-renesas-soc@vger.kernel.org
> >> To: devicetree@vger.kernel.org
> >> ---
> >> drivers/of/address.c | 9 +++++----
> >> 1 file changed, 5 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/of/address.c b/drivers/of/address.c
> >> index b492176c0572..ae2819e148b8 100644
> >> --- a/drivers/of/address.c
> >> +++ b/drivers/of/address.c
> >> @@ -616,7 +616,7 @@ static struct of_bus *of_match_bus(struct device_node *np)
> >> return NULL;
> >> }
> >>
> >> -static int of_empty_ranges_quirk(struct device_node *np)
> >> +static bool of_empty_ranges_quirk(struct device_node *np)
> >> {
> >> if (IS_ENABLED(CONFIG_PPC)) {
> >> /* To save cycles, we cache the result for global "Mac" setting */
> >> @@ -631,7 +631,8 @@ static int of_empty_ranges_quirk(struct device_node *np)
> >> quirk_state =
> >> of_machine_is_compatible("Power Macintosh") ||
> >> of_machine_is_compatible("MacRISC");
> >> - return quirk_state;
> >> + if (quirk_state > 0)
> >> + return true;
> >> }
> >> return false;
> >> }
> >> @@ -662,8 +663,8 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
> >> * This code is only enabled on powerpc. --gcl
> >> */
> >> ranges = of_get_property(parent, rprop, &rlen);
> >> - if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> >> - pr_debug("no ranges; cannot translate\n");
> >> + if (ranges == NULL && of_empty_ranges_quirk(parent)) {
> >> + pr_err("no ranges; cannot translate\n");
> >
> > This is wrong. If you have NULL ranges and not the quirk, then no
> > ranges is an error. IOW, if you are getting an error here, you have an
> > error in your DT (because I assume you are not working on a PASemi or
> > Apple system).
>
> The way I understand the code is that
> if (you have no ranges in the DT) AND (the quirk is applicable) then
> print the message. Which is what this patch does.
Your understanding is wrong.
> Am I missing something ?
The normal case is you must have ranges to translate addresses. If you
don't have ranges (say for I2C addresses), then you shouldn't be in
this code. The quirk is for when there is not a ranges property but
should be. IOW, if the quirk is true, then pretend there is an empty
ranges (1:1 translation) property and continue to translate the
address.
Rob
^ permalink raw reply
* Re: [PATCH] ARM: dts: imx7d: sbc-iot-imx7: add recovery for i2c3/4
From: André Draszik @ 2019-08-10 21:58 UTC (permalink / raw)
To: Philippe Schenker
Cc: André Draszik, Max Krummenacher, Oleksandr Suvorov,
Marcel Ziswiler, Rob Herring, Mark Rutland, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP Linux Team, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20190807082556.5013-6-philippe.schenker@toradex.com>
On Wed, 07 Aug 2019 08:26:15 +0000, Philippe Schenker wrote:
> From: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
>
> - add recovery mode for applicable i2c buses for
> Colibri iMX7 module.
>
> Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
> Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
> ---
>
> Changes in v3: None
> Changes in v2: None
>
> arch/arm/boot/dts/imx7-colibri.dtsi | 25 +++++++++++++++++++++++--
> 1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/boot/dts/imx7-colibri.dtsi b/arch/arm/boot/dts/imx7-colibri.dtsi
> index a8d992f3e897..2480623c92ff 100644
> --- a/arch/arm/boot/dts/imx7-colibri.dtsi
> +++ b/arch/arm/boot/dts/imx7-colibri.dtsi
> @@ -140,8 +140,12 @@
>
> &i2c1 {
> clock-frequency = <100000>;
> - pinctrl-names = "default";
> + pinctrl-names = "default", "gpio";
> pinctrl-0 = <&pinctrl_i2c1 &pinctrl_i2c1_int>;
> + pinctrl-1 = <&pinctrl_i2c1_recovery &pinctrl_i2c1_int>;
> + scl-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
scl-gpios should be (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN) since
commit d2d0ad2aec4a ("i2c: imx: use open drain for recovery GPIO")
Otherwise you'll get a boot-time warning:
enforced open drain please flag it properly in DT/ACPI DSDT/board file
> + sda-gpios = <&gpio1 5 GPIO_ACTIVE_HIGH>;
> +
> status = "okay";
>
> codec: sgtl5000@a {
> @@ -242,8 +246,11 @@
>
> &i2c4 {
> clock-frequency = <100000>;
> - pinctrl-names = "default";
> + pinctrl-names = "default", "gpio";
> pinctrl-0 = <&pinctrl_i2c4>;
> + pinctrl-1 = <&pinctrl_i2c4_recovery>;
> + scl-gpios = <&gpio7 8 GPIO_ACTIVE_HIGH>;
and here, too.
Cheers,
André
^ permalink raw reply
* RE: [PATCH v9 5/6] usb:cdns3 Add Cadence USB3 DRD Driver
From: Pawel Laszczak @ 2019-08-10 20:39 UTC (permalink / raw)
To: Felipe Balbi, devicetree@vger.kernel.org
Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
hdegoede@redhat.com, heikki.krogerus@linux.intel.com,
robh+dt@kernel.org, rogerq@ti.com, linux-kernel@vger.kernel.org,
jbergsagel@ti.com, nsekhar@ti.com, nm@ti.com, Suresh Punnoose,
peter.chen@nxp.com, Jayshri Dajiram Pawar, Rahul Kumar
In-Reply-To: <8736idnu0q.fsf@gmail.com>
Hi,
>
>Pawel Laszczak <pawell@cadence.com> writes:
>>>> +static int cdns3_gadget_start(struct cdns3 *cdns)
>>>> +{
>>>> + struct cdns3_device *priv_dev;
>>>> + u32 max_speed;
>>>> + int ret;
>>>> +
>>>> + priv_dev = kzalloc(sizeof(*priv_dev), GFP_KERNEL);
>>>> + if (!priv_dev)
>>>> + return -ENOMEM;
>>>> +
>>>> + cdns->gadget_dev = priv_dev;
>>>> + priv_dev->sysdev = cdns->dev;
>>>> + priv_dev->dev = cdns->dev;
>>>> + priv_dev->regs = cdns->dev_regs;
>>>> +
>>>> + device_property_read_u16(priv_dev->dev, "cdns,on-chip-buff-size",
>>>> + &priv_dev->onchip_buffers);
>>>> +
>>>> + if (priv_dev->onchip_buffers <= 0) {
>>>> + u32 reg = readl(&priv_dev->regs->usb_cap2);
>>>> +
>>>> + priv_dev->onchip_buffers = USB_CAP2_ACTUAL_MEM_SIZE(reg);
>>>> + }
>>>> +
>>>> + if (!priv_dev->onchip_buffers)
>>>> + priv_dev->onchip_buffers = 256;
>>>> +
>>>> + max_speed = usb_get_maximum_speed(cdns->dev);
>>>> +
>>>> + /* Check the maximum_speed parameter */
>>>> + switch (max_speed) {
>>>> + case USB_SPEED_FULL:
>>>> + case USB_SPEED_HIGH:
>>>> + case USB_SPEED_SUPER:
>>>> + break;
>>>> + default:
>>>> + dev_err(cdns->dev, "invalid maximum_speed parameter %d\n",
>>>> + max_speed);
>>>> + /* fall through */
>>>> + case USB_SPEED_UNKNOWN:
>>>> + /* default to superspeed */
>>>> + max_speed = USB_SPEED_SUPER;
>>>> + break;
>>>> + }
>>>> +
>>>> + /* fill gadget fields */
>>>> + priv_dev->gadget.max_speed = max_speed;
>>>> + priv_dev->gadget.speed = USB_SPEED_UNKNOWN;
>>>> + priv_dev->gadget.ops = &cdns3_gadget_ops;
>>>> + priv_dev->gadget.name = "usb-ss-gadget";
>>>> + priv_dev->gadget.sg_supported = 1;
>>>> +
>>>> + spin_lock_init(&priv_dev->lock);
>>>> + INIT_WORK(&priv_dev->pending_status_wq,
>>>> + cdns3_pending_setup_status_handler);
>>>> +
>>>> + /* initialize endpoint container */
>>>> + INIT_LIST_HEAD(&priv_dev->gadget.ep_list);
>>>> + INIT_LIST_HEAD(&priv_dev->aligned_buf_list);
>>>> +
>>>> + ret = cdns3_init_eps(priv_dev);
>>>> + if (ret) {
>>>> + dev_err(priv_dev->dev, "Failed to create endpoints\n");
>>>> + goto err1;
>>>> + }
>>>> +
>>>> + /* allocate memory for setup packet buffer */
>>>> + priv_dev->setup_buf = dma_alloc_coherent(priv_dev->sysdev, 8,
>>>> + &priv_dev->setup_dma, GFP_DMA);
>>>> + if (!priv_dev->setup_buf) {
>>>> + ret = -ENOMEM;
>>>> + goto err2;
>>>> + }
>>>> +
>>>> + priv_dev->dev_ver = readl(&priv_dev->regs->usb_cap6);
>>>> +
>>>> + dev_dbg(priv_dev->dev, "Device Controller version: %08x\n",
>>>> + readl(&priv_dev->regs->usb_cap6));
>>>> + dev_dbg(priv_dev->dev, "USB Capabilities:: %08x\n",
>>>> + readl(&priv_dev->regs->usb_cap1));
>>>> + dev_dbg(priv_dev->dev, "On-Chip memory cnfiguration: %08x\n",
>>>> + readl(&priv_dev->regs->usb_cap2));
>>>> +
>>>> + priv_dev->dev_ver = GET_DEV_BASE_VERSION(priv_dev->dev_ver);
>>>> +
>>>> + priv_dev->zlp_buf = kzalloc(CDNS3_EP_ZLP_BUF_SIZE, GFP_KERNEL);
>>>> + if (!priv_dev->zlp_buf) {
>>>> + ret = -ENOMEM;
>>>> + goto err3;
>>>> + }
>>>> +
>>>> + /* add USB gadget device */
>>>> + ret = usb_add_gadget_udc(priv_dev->dev, &priv_dev->gadget);
>>>> + if (ret < 0) {
>>>> + dev_err(priv_dev->dev,
>>>> + "Failed to register USB device controller\n");
>>>> + goto err4;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +err4:
>>>> + kfree(priv_dev->zlp_buf);
>>>> +err3:
>>>> + dma_free_coherent(priv_dev->sysdev, 8, priv_dev->setup_buf,
>>>> + priv_dev->setup_dma);
>>>> +err2:
>>>> + cdns3_free_all_eps(priv_dev);
>>>> +err1:
>>>> + cdns->gadget_dev = NULL;
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int __cdns3_gadget_init(struct cdns3 *cdns)
>>>> +{
>>>> + struct cdns3_device *priv_dev;
>>>> + int ret = 0;
>>>> +
>>>> + cdns3_drd_switch_gadget(cdns, 1);
>>>> + pm_runtime_get_sync(cdns->dev);
>>>> +
>>>> + ret = cdns3_gadget_start(cdns);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + priv_dev = cdns->gadget_dev;
>>>> + ret = devm_request_threaded_irq(cdns->dev, cdns->dev_irq,
>>>> + cdns3_device_irq_handler,
>>>> + cdns3_device_thread_irq_handler,
>>>> + IRQF_SHARED, dev_name(cdns->dev), cdns);
>>>
>>>copied handlers here for commenting. Note that you don't have
>>>IRQF_ONESHOT:
>>
>> I know, I can't use IRQF_ ONESHOT flag in this case. I have implemented
>> some code for masking/unmasking interrupts in cdns3_device_irq_handler.
>>
>> Some priority interrupts should be handled ASAP so I can't blocked interrupt
>> Line.
>
>You're completely missing my comment. Your top half should be as short
>as possile. It should only check if current device generated
>interrupts. If it did, then you should wake the thread handler.
>
>This is to improve realtime behavior but not keeping preemption disabled
>for longer than necessary.
Ok, I understand. I will move it to thread handler.
I can't use IRQF_ONESHOT flag because it doesn't work when interrupt line is shared.
I have such situation in which one interrupt line is shared with ehci and cdns3 driver.
In such case this function returns error code.
So probably I will need to mask only the reported interrupts.
I can't mask all interrupt using controller register because I can miss some of them.
After masking all interrupt the next new event will not be reported in usb_ists, ep_ists
registers.
>
>>>> +static irqreturn_t cdns3_device_irq_handler(int irq, void *data)
>>>> +{
>>>> + struct cdns3_device *priv_dev;
>>>> + struct cdns3 *cdns = data;
>>>> + irqreturn_t ret = IRQ_NONE;
>>>> + unsigned long flags;
>>>> + u32 reg;
>>>> +
>>>> + priv_dev = cdns->gadget_dev;
>>>> + spin_lock_irqsave(&priv_dev->lock, flags);
>>>
>>>the top half handler runs in hardirq context. You don't need any locks
>>>here. Also IRQs are *already* disabled, you don't need to disable them again.
>>
>> I will remove spin_lock_irqsave but I need to disable only some of the interrupts.
>> I disable interrupts associated with USB endpoints. Handling of them can be
>> deferred to thread handled.
>
>you should defer all of them to thread. Endpoints or otherwise.
I will do this.
Also I remove spin_lock_irqsave(&priv_dev->lock, flags);
As I remember it's not needed here.
>
>>>> +
>>>> + /* check USB device interrupt */
>>>> + reg = readl(&priv_dev->regs->usb_ists);
>>>> +
>>>> + if (reg) {
>>>> + writel(reg, &priv_dev->regs->usb_ists);
>>>> + cdns3_check_usb_interrupt_proceed(priv_dev, reg);
>>>> + ret = IRQ_HANDLED;
>>>
>>>now, because you _don't_ mask this interrupt, you're gonna have
>>>issues. Say we actually get both device and endpoint interrupts while
>>>the thread is already running with previous endpoint interrupts. Now
>>>we're gonna reenter the top half, because device interrupts are *not*
>>>masked, which will read usb_ists and handle it here.
>>
>> Endpoint interrupts are masked in cdns3_device_irq_handler and stay masked
>> until they are not handled in threaded handler.
>
>Quick question, then: these ISTS registers, are they masked interrupt
>status or raw interrupt status?
Yes it's masked, but after masking them the new interrupts will not be reported
In ISTS registers. Form this reason I can mask only reported interrupt.
>
>> Of course, not all endpoint interrupts are masked, but only reported in ep_ists.
>> USB interrupt will be handled immediately.
>>
>> Also, I can get next endpoint interrupt from not masked endpoint and driver also again wake
>> the thread. I saw such situation, but threaded interrupt handler has been working correct
>> in such situations.
>>
>> In thread handler driver checks again which endpoint should be handled in ep_ists.
>>
>> I think that such situation should also occurs during our LPM enter/exit test.
>> So, driver has been tested for such case. During this test driver during
>> transferring data generate a huge number of LPM interrupts which
>> are usb interrupts.
>>
>> I can't block usb interrupts interrupts because:
>> /*
>> * WORKAROUND: CDNS3 controller has issue with hardware resuming
>> * from L1. To fix it, if any DMA transfer is pending driver
>> * must starts driving resume signal immediately.
>> */
>
>I can't see why this would prevent you from defering handling to thread
>handler.
>
I also will try to move it, but this change can has impact on performance.
>
>>>> + if (priv_dev->run_garbage_colector) {
>>>
>>>wait, what?
>>
>> DMA require data buffer aligned to 8 bytes. So, if buffer data is not aligned
>> driver allocate aligned buffer for data and copy it from unaligned to
>> Aligned.
>>
>>>
>>>ps: correct spelling is "collector" ;-)
>>
>> Ok, thanks.
>>>
>>>> + struct cdns3_aligned_buf *buf, *tmp;
>>>> +
>>>> + list_for_each_entry_safe(buf, tmp, &priv_dev->aligned_buf_list,
>>>> + list) {
>>>> + if (!buf->in_use) {
>>>> + list_del(&buf->list);
>>>> +
>>>> + spin_unlock_irqrestore(&priv_dev->lock, flags);
>>>
>>>creates the possibility of a race condition
>> Why? In this place the buf can't be used.
>
>but you're reenabling interrupts, right?
Yes, driver frees not used buffers here.
I think that it's the safest place for this purpose.
>
>>>> + dma_free_coherent(priv_dev->sysdev, buf->size,
>>>> + buf->buf,
>>>> + buf->dma);
>>>> + spin_lock_irqsave(&priv_dev->lock, flags);
>>>> +
>>>> + kfree(buf);
>>>
>>>why do you even need this "garbage collector"?
>>
>> I need to free not used memory. The once allocated buffer will be associated with
>> request, but if request.length will be increased in usb_request then driver will
>> must allocate the bigger buffer. As I remember I couldn't call dma_free_coherent
>> in interrupt context so I had to move it to thread handled. This flag was used to avoid
>> going through whole aligned_buf_list every time.
>> In most cases this part will never called int this place
>
>Did you try, btw, setting the quirk flag which tells gadget drivers to
>always allocate buffers aligned to MaxPacketSize? Wouldn't that be enough?
If found only quirk_ep_out_aligned_size flag, but it align only buffer size.
DMA used by this controller must have buffer address aligned to 8.
I think that on most architecture kmalloc should guarantee such aligned.
The problem was detected on NXP testing board.
On my board all buffer address are alignment at least to 8.
>
>>>> + TP_printk("%s: req: %p, req buff %p, length: %u/%u %s%s%s, status: %d,"
>>>> + cd " trb: [start:%d, end:%d: virt addr %pa], flags:%x ",
>>>> + __get_str(name), __entry->req, __entry->buf, __entry->actual,
>>>> + __entry->length,
>>>> + __entry->zero ? "zero | " : "",
>>>> + __entry->short_not_ok ? "short | " : "",
>>>> + __entry->no_interrupt ? "no int" : "",
>>>
>>>I guess you didn't really think the formatting through. Think about what
>>>happens if you get a request with only zero flag or only short flag. How
>>>will this log look like?
>>
>> Like this:
>> cdns3_gadget_giveback: ep0: req: 0000000071a6a5f5, req buff 000000008d40c4db, length: 60/60 zero | , status: 0, trb: [start:0, end:0:
>virt addr (null)], flags:0
>>
>> Is it something wrong with this?. Maybe one extra sign |.
>
>yes, the extra | :-)
>
>This is one reason why I switched to character flags where a lower case
>character means flag is cleared while uppercase means it's set.
I've made it in this way in v10
--
Thanks
Pawell
^ permalink raw reply
* Re: [PATCH] of: Fix of_empty_ranges_quirk()
From: Frank Rowand @ 2019-08-10 19:47 UTC (permalink / raw)
To: Marek Vasut, Rob Herring
Cc: devicetree, Marek Vasut,
open list:MEDIA DRIVERS FOR RENESAS - FCP
In-Reply-To: <10818888-6476-f4b1-1a2e-e10c3159327f@gmail.com>
On 8/10/19 6:39 AM, Marek Vasut wrote:
> On 8/10/19 12:34 AM, Rob Herring wrote:
>> On Fri, Aug 9, 2019 at 11:33 AM <marek.vasut@gmail.com> wrote:
>>>
>>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>
>>> The of_empty_ranges_quirk() returns a mix of boolean and signed integer
>>> types, which cannot work well.
>>
>> It never returns a negative. The negative is used as an uninitialized
>> flag. Note quirk_state is static.
>
> It's still mixing boolean and signed int types though, which isn't right.
>From a code readability aspect, Marek is correct.
The code author used "stupid (or clever) coding tricks" (tm) to save a
little bit of memory. A more readable implementation would be:
static bool of_empty_ranges_quirk(struct device_node *np)
{
/*
* As far as we know, the missing "ranges" problem only exists on Apple
* machines, so only enable the exception on powerpc. --gcl
*/
if (IS_ENABLED(CONFIG_PPC)) {
/* Cache the result for global "Mac" setting */
static int quirk_state_initialized = 0;
static bool quirk_state;
/* PA-SEMI sdc DT bug */
if (of_device_is_compatible(np, "1682m-sdc"))
return true;
if (!quirk_state_initialized)
quirk_state_initialized = 1;
quirk_state =
of_machine_is_compatible("Power Macintosh") ||
of_machine_is_compatible("MacRISC");
return quirk_state;
}
return false;
}
I would also rename of_empty_ranges_quirk() to something like
of_missing_ranges_is_ok() or of_missing_ranges_allowed().
"quirk" does not convey any useful information while my proposed rename
describes what the function is actually checking for.
The comment that I added is currently in the caller of of_empty_ranges_quirk(),
but instead belongs in of_empty_ranges_quirk(). When I read that comment in
of_translate_one(), my reaction was to look for the check for powerpc in
of_translate_one() and to be puzzled when I could not find it. I also
modified the comment for the changed context. Thus the "--gcl" portion
of the comment should also be removed from of_translate_one().
The more readable implementation (IMNSHO) uses slightly more memory and
slightly more code, but it is more direct about what it is doing and thus
more readable.
-Frank
>
>>> Replace that with boolean only and fix
>>> usage logic in of_translate_one() -- the check should trigger when the
>>> ranges are NULL and the quirk is applicable on the hardware.
>>>
>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: Frank Rowand <frowand.list@gmail.com>
>>> Cc: linux-renesas-soc@vger.kernel.org
>>> To: devicetree@vger.kernel.org
>>> ---
>>> drivers/of/address.c | 9 +++++----
>>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/of/address.c b/drivers/of/address.c
>>> index b492176c0572..ae2819e148b8 100644
>>> --- a/drivers/of/address.c
>>> +++ b/drivers/of/address.c
>>> @@ -616,7 +616,7 @@ static struct of_bus *of_match_bus(struct device_node *np)
>>> return NULL;
>>> }
>>>
>>> -static int of_empty_ranges_quirk(struct device_node *np)
>>> +static bool of_empty_ranges_quirk(struct device_node *np)
>>> {
>>> if (IS_ENABLED(CONFIG_PPC)) {
>>> /* To save cycles, we cache the result for global "Mac" setting */
>>> @@ -631,7 +631,8 @@ static int of_empty_ranges_quirk(struct device_node *np)
>>> quirk_state =
>>> of_machine_is_compatible("Power Macintosh") ||
>>> of_machine_is_compatible("MacRISC");
>>> - return quirk_state;
>>> + if (quirk_state > 0)
>>> + return true;
>>> }
>>> return false;
>>> }
>>> @@ -662,8 +663,8 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>>> * This code is only enabled on powerpc. --gcl
>>> */
>>> ranges = of_get_property(parent, rprop, &rlen);
>>> - if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
>>> - pr_debug("no ranges; cannot translate\n");
>>> + if (ranges == NULL && of_empty_ranges_quirk(parent)) {
>>> + pr_err("no ranges; cannot translate\n");
>>
>> This is wrong. If you have NULL ranges and not the quirk, then no
>> ranges is an error. IOW, if you are getting an error here, you have an
>> error in your DT (because I assume you are not working on a PASemi or
>> Apple system).
>
> The way I understand the code is that
> if (you have no ranges in the DT) AND (the quirk is applicable) then
> print the message. Which is what this patch does.
>
> Am I missing something ?
>
^ permalink raw reply
* Re: [PATCH v11 09/12] soc: mediatek: cmdq: define the instruction struct
From: houlong wei @ 2019-08-10 16:38 UTC (permalink / raw)
To: Bibby Hsieh (謝濟遠)
Cc: devicetree@vger.kernel.org, Nicolas Boichat, Philipp Zabel,
srv_heupstream, Daoyuan Huang (黃道原),
Sascha Hauer, Jassi Brar, linux-kernel@vger.kernel.org,
Daniel Kurtz, houlong.wei, YT Shen (沈岳霆),
CK Hu (胡俊光), Rob Herring,
linux-mediatek@lists.infradead.org,
Ginny Chen (陳治傑), Sascha Hauer
In-Reply-To: <1565453520.19079.17.camel@mhfsdcap03>
On Sun, 2019-08-11 at 00:12 +0800, houlong wei wrote:
> Hi Bibby, I have inline comment in function cmdq_pkt_write_mask().
>
> On Mon, 2019-07-29 at 15:01 +0800, Bibby Hsieh wrote:
> > Define an instruction structure for gce driver to append command.
> > This structure can make the client's code more readability.
> >
> > Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> > Reviewed-by: CK Hu <ck.hu@mediatek.com>
> > ---
> > drivers/soc/mediatek/mtk-cmdq-helper.c | 103 +++++++++++++++--------
> > include/linux/mailbox/mtk-cmdq-mailbox.h | 2 +
> > 2 files changed, 72 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index 7aa0517ff2f3..0886c4967ca4 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -9,12 +9,24 @@
> > #include <linux/mailbox_controller.h>
> > #include <linux/soc/mediatek/mtk-cmdq.h>
[...]
> >
> > int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys,
> > u16 offset, u32 value, u32 mask)
> > {
> > + struct cmdq_instruction *inst;
> > u32 offset_mask = offset;
> > - int err = 0;
> >
> > if (mask != 0xffffffff) {
> > - err = cmdq_pkt_append_command(pkt, CMDQ_CODE_MASK, 0, ~mask);
> > + inst = cmdq_pkt_append_command(pkt);
> > + if (!inst)
> > + return -ENOMEM;
> > +
> > + inst->op = CMDQ_CODE_MASK;
> > + inst->mask = ~mask;
> > offset_mask |= CMDQ_WRITE_ENABLE_MASK;
> > }
> > - err |= cmdq_pkt_write(pkt, value, subsys, offset_mask);
> >
> > - return err;
> > + return cmdq_pkt_write(pkt, subsys, offset_mask, value);
We need add a type conversion here, (u8)offset_mask, for your new
function type. Er... it's better to remove local variable 'offset_mask'
and replace it with 'offset'.
> > }
[...]
^ permalink raw reply
* Re: [PATCH v11 12/12] arm64: dts: add gce node for mt8183
From: houlong wei @ 2019-08-10 16:25 UTC (permalink / raw)
To: Bibby Hsieh
Cc: Jassi Brar, Matthias Brugger, Rob Herring,
CK Hu (胡俊光), Daniel Kurtz, Sascha Hauer,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-mediatek@lists.infradead.org, srv_heupstream, Sascha Hauer,
Philipp Zabel, Nicolas Boichat
In-Reply-To: <20190729070106.9332-13-bibby.hsieh@mediatek.com>
On Mon, 2019-07-29 at 15:01 +0800, Bibby Hsieh wrote:
> add gce device node for mt8183
>
> Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> ---
> arch/arm64/boot/dts/mediatek/mt8183.dtsi | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> index 08274bfcebd8..98d17d0bdebf 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> @@ -8,6 +8,7 @@
> #include <dt-bindings/clock/mt8183-clk.h>
> #include <dt-bindings/interrupt-controller/arm-gic.h>
> #include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/gce/mt8183-gce.h>
>
> / {
> compatible = "mediatek,mt8183";
> @@ -212,6 +213,15 @@
> clock-names = "spi", "wrap";
> };
>
> + gce: gce@10238000 {
Use mailbox@ instead of gce@, Rob suggested in
https://patchwork.kernel.org/patch/10491213/
> + compatible = "mediatek,mt8183-gce";
> + reg = <0 0x10238000 0 0x4000>;
> + interrupts = <GIC_SPI 162 IRQ_TYPE_LEVEL_LOW>;
> + #mbox-cells = <3>;
> + clocks = <&infracfg CLK_INFRA_GCE>;
> + clock-names = "gce";
> + };
> +
> uart0: serial@11002000 {
> compatible = "mediatek,mt8183-uart",
> "mediatek,mt6577-uart";
^ permalink raw reply
* Re: [PATCH v11 11/12] soc: mediatek: cmdq: add cmdq_dev_get_client_reg function
From: houlong wei @ 2019-08-10 16:18 UTC (permalink / raw)
To: Bibby Hsieh
Cc: devicetree@vger.kernel.org, Nicolas Boichat, Philipp Zabel,
srv_heupstream, Daoyuan Huang (黃道原),
Sascha Hauer, Jassi Brar, linux-kernel@vger.kernel.org,
Daniel Kurtz, houlong.wei, YT Shen (沈岳霆),
CK Hu (胡俊光), Rob Herring,
linux-mediatek@lists.infradead.org,
Ginny Chen (陳治傑), Sascha Hauer
In-Reply-To: <20190729070106.9332-12-bibby.hsieh@mediatek.com>
On Mon, 2019-07-29 at 15:01 +0800, Bibby Hsieh wrote:
> GCE cannot know the register base address, this function
> can help cmdq client to get the cmdq_client_reg structure.
>
> Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> ---
> drivers/soc/mediatek/mtk-cmdq-helper.c | 29 ++++++++++++++++++++++++++
> include/linux/soc/mediatek/mtk-cmdq.h | 21 +++++++++++++++++++
> 2 files changed, 50 insertions(+)
>
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index 70ad4d806fac..9695b75cfc89 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -27,6 +27,35 @@ struct cmdq_instruction {
> u8 op;
> };
>
> +int cmdq_dev_get_client_reg(struct device *dev,
> + struct cmdq_client_reg *client_reg, int idx)
> +{
> + struct of_phandle_args spec;
> + int err;
> +
> + if (!client_reg)
> + return -ENOENT;
> +
> + err = of_parse_phandle_with_fixed_args(dev->of_node,
> + "mediatek,gce-client-reg",
> + 3, idx, &spec);
> + if (err < 0) {
> + dev_err(dev,
> + "error %d can't parse gce-client-reg property (%d)",
> + err, idx);
> +
> + return err;
> + }
> +
> + client_reg->subsys = spec.args[0];
> + client_reg->offset = spec.args[1];
> + client_reg->size = spec.args[2];
Maybe we need add type conversion to avoid compiling warnings.
client_reg->subsys = (u8)spec.args[0];
client_reg->offset = (u16)spec.args[1];
client_reg->size = (u16)spec.args[2];
> + of_node_put(spec.np);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(cmdq_dev_get_client_reg);
> +
> static void cmdq_client_timeout(struct timer_list *t)
> {
> struct cmdq_client *client = from_timer(client, t, timer);
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> index a345870a6d10..be402c4c740e 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -15,6 +15,12 @@
>
> struct cmdq_pkt;
>
> +struct cmdq_client_reg {
> + u8 subsys;
> + u16 offset;
> + u16 size;
> +};
> +
> struct cmdq_client {
> spinlock_t lock;
> u32 pkt_cnt;
> @@ -142,4 +148,19 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
> */
> int cmdq_pkt_flush(struct cmdq_pkt *pkt);
>
> +/**
> + * cmdq_dev_get_client_reg() - parse cmdq client reg from the device
> + * node of CMDQ client
> + * @dev: device of CMDQ mailbox clienti
> + * @client_reg: CMDQ client reg pointer
> + * @idx: the index of desired reg
> + *
> + * Return: 0 for success; else the error code is returned
> + *
> + * Help CMDQ client pasing the cmdq client reg
> + * from the device node of CMDQ client.
> + */
> +int cmdq_dev_get_client_reg(struct device *dev,
> + struct cmdq_client_reg *client_reg, int idx);
> +
Could we keep the same order of function declaration and implementation?
> #endif /* __MTK_CMDQ_H__ */
^ permalink raw reply
* Re: [PATCH v11 09/12] soc: mediatek: cmdq: define the instruction struct
From: houlong wei @ 2019-08-10 16:12 UTC (permalink / raw)
To: Bibby Hsieh
Cc: Jassi Brar, Matthias Brugger, Rob Herring,
CK Hu (胡俊光), Daniel Kurtz, Sascha Hauer,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-mediatek@lists.infradead.org, srv_heupstream, Sascha Hauer,
Philipp Zabel, Nicolas Boichat
In-Reply-To: <20190729070106.9332-10-bibby.hsieh@mediatek.com>
Hi Bibby, I have inline comment in function cmdq_pkt_write_mask().
On Mon, 2019-07-29 at 15:01 +0800, Bibby Hsieh wrote:
> Define an instruction structure for gce driver to append command.
> This structure can make the client's code more readability.
>
> Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> Reviewed-by: CK Hu <ck.hu@mediatek.com>
> ---
> drivers/soc/mediatek/mtk-cmdq-helper.c | 103 +++++++++++++++--------
> include/linux/mailbox/mtk-cmdq-mailbox.h | 2 +
> 2 files changed, 72 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index 7aa0517ff2f3..0886c4967ca4 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -9,12 +9,24 @@
> #include <linux/mailbox_controller.h>
> #include <linux/soc/mediatek/mtk-cmdq.h>
>
> -#define CMDQ_ARG_A_WRITE_MASK 0xffff
> #define CMDQ_WRITE_ENABLE_MASK BIT(0)
> #define CMDQ_EOC_IRQ_EN BIT(0)
> #define CMDQ_EOC_CMD ((u64)((CMDQ_CODE_EOC << CMDQ_OP_CODE_SHIFT)) \
> << 32 | CMDQ_EOC_IRQ_EN)
>
> +struct cmdq_instruction {
> + union {
> + u32 value;
> + u32 mask;
> + };
> + union {
> + u16 offset;
> + u16 event;
> + };
> + u8 subsys;
> + u8 op;
> +};
> +
> static void cmdq_client_timeout(struct timer_list *t)
> {
> struct cmdq_client *client = from_timer(client, t, timer);
> @@ -110,10 +122,8 @@ void cmdq_pkt_destroy(struct cmdq_pkt *pkt)
> }
> EXPORT_SYMBOL(cmdq_pkt_destroy);
>
> -static int cmdq_pkt_append_command(struct cmdq_pkt *pkt, enum cmdq_code code,
> - u32 arg_a, u32 arg_b)
> +static struct cmdq_instruction *cmdq_pkt_append_command(struct cmdq_pkt *pkt)
> {
> - u64 *cmd_ptr;
>
> if (unlikely(pkt->cmd_buf_size + CMDQ_INST_SIZE > pkt->buf_size)) {
> /*
> @@ -127,81 +137,108 @@ static int cmdq_pkt_append_command(struct cmdq_pkt *pkt, enum cmdq_code code,
> pkt->cmd_buf_size += CMDQ_INST_SIZE;
> WARN_ONCE(1, "%s: buffer size %u is too small !\n",
> __func__, (u32)pkt->buf_size);
> - return -ENOMEM;
> + return NULL;
> }
> - cmd_ptr = pkt->va_base + pkt->cmd_buf_size;
> - (*cmd_ptr) = (u64)((code << CMDQ_OP_CODE_SHIFT) | arg_a) << 32 | arg_b;
> +
> pkt->cmd_buf_size += CMDQ_INST_SIZE;
>
> - return 0;
> + return pkt->va_base + pkt->cmd_buf_size - CMDQ_INST_SIZE;
> }
>
> int cmdq_pkt_write(struct cmdq_pkt *pkt, u8 subsys, u16 offset, u32 value)
> {
> - u32 arg_a = (offset & CMDQ_ARG_A_WRITE_MASK) |
> - (subsys << CMDQ_SUBSYS_SHIFT);
> + struct cmdq_instruction *inst;
> +
> + inst = cmdq_pkt_append_command(pkt);
> + if (!inst)
> + return -ENOMEM;
> +
> + inst->op = CMDQ_CODE_WRITE;
> + inst->value = value;
> + inst->offset = offset;
> + inst->subsys = subsys;
>
> - return cmdq_pkt_append_command(pkt, CMDQ_CODE_WRITE, arg_a, value);
> + return 0;
> }
> EXPORT_SYMBOL(cmdq_pkt_write);
>
> int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys,
> u16 offset, u32 value, u32 mask)
> {
> + struct cmdq_instruction *inst;
> u32 offset_mask = offset;
> - int err = 0;
>
> if (mask != 0xffffffff) {
> - err = cmdq_pkt_append_command(pkt, CMDQ_CODE_MASK, 0, ~mask);
> + inst = cmdq_pkt_append_command(pkt);
> + if (!inst)
> + return -ENOMEM;
> +
> + inst->op = CMDQ_CODE_MASK;
> + inst->mask = ~mask;
There were some discussion about how to reduce cmdq buffer allocation
times reuse a cmdq packet.Please refer to the below links.
https://patchwork.kernel.org/patch/10193349/
https://patchwork.kernel.org/patch/10491161/
If we reuse a cmdq packet, we get the cmdq instruction buffer which may
contains previous data. To generate correct instructions, we may need
consider how to clear the previous data.
1.Set all members of a cmdq_instruction instance.
e.g. Add two lines of code below for this case.
inst->offset = 0;
inst->subsys = 0;
2. Before reuse a packet, do memset() for the command buffer.
How do you think about it?
> offset_mask |= CMDQ_WRITE_ENABLE_MASK;
> }
> - err |= cmdq_pkt_write(pkt, value, subsys, offset_mask);
>
> - return err;
> + return cmdq_pkt_write(pkt, subsys, offset_mask, value);
> }
> EXPORT_SYMBOL(cmdq_pkt_write_mask);
>
> int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event)
> {
> - u32 arg_b;
> + struct cmdq_instruction *inst;
>
> if (event >= CMDQ_MAX_EVENT)
> return -EINVAL;
>
> - /*
> - * WFE arg_b
> - * bit 0-11: wait value
> - * bit 15: 1 - wait, 0 - no wait
> - * bit 16-27: update value
> - * bit 31: 1 - update, 0 - no update
> - */
> - arg_b = CMDQ_WFE_UPDATE | CMDQ_WFE_WAIT | CMDQ_WFE_WAIT_VALUE;
> + inst = cmdq_pkt_append_command(pkt);
> + if (!inst)
> + return -ENOMEM;
> +
> + inst->op = CMDQ_CODE_WFE;
> + inst->value = CMDQ_WFE_OPTION;
> + inst->event = event;
>
> - return cmdq_pkt_append_command(pkt, CMDQ_CODE_WFE, event, arg_b);
> + return 0;
> }
> EXPORT_SYMBOL(cmdq_pkt_wfe);
>
> int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event)
> {
> + struct cmdq_instruction *inst;
> +
> if (event >= CMDQ_MAX_EVENT)
> return -EINVAL;
>
> - return cmdq_pkt_append_command(pkt, CMDQ_CODE_WFE, event,
> - CMDQ_WFE_UPDATE);
> + inst = cmdq_pkt_append_command(pkt);
> + if (!inst)
> + return -ENOMEM;
> +
> + inst->op = CMDQ_CODE_WFE;
> + inst->value = CMDQ_WFE_UPDATE;
> + inst->event = event;
> +
> + return 0;
> }
> EXPORT_SYMBOL(cmdq_pkt_clear_event);
>
> static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> {
> - int err;
> + struct cmdq_instruction *inst;
> +
> + inst = cmdq_pkt_append_command(pkt);
> + if (!inst)
> + return -ENOMEM;
>
> - /* insert EOC and generate IRQ for each command iteration */
> - err = cmdq_pkt_append_command(pkt, CMDQ_CODE_EOC, 0, CMDQ_EOC_IRQ_EN);
> + inst->op = CMDQ_CODE_EOC;
> + inst->value = CMDQ_EOC_IRQ_EN;
>
> - /* JUMP to end */
> - err |= cmdq_pkt_append_command(pkt, CMDQ_CODE_JUMP, 0, CMDQ_JUMP_PASS);
> + inst = cmdq_pkt_append_command(pkt);
> + if (!inst)
> + return -ENOMEM;
> +
> + inst->op = CMDQ_CODE_JUMP;
> + inst->value = CMDQ_JUMP_PASS;
>
> - return err;
> + return 0;
> }
>
> static void cmdq_pkt_flush_async_cb(struct cmdq_cb_data data)
> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> index 911475da7a53..c8adedefaf42 100644
> --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> @@ -19,6 +19,8 @@
> #define CMDQ_WFE_UPDATE BIT(31)
> #define CMDQ_WFE_WAIT BIT(15)
> #define CMDQ_WFE_WAIT_VALUE 0x1
> +#define CMDQ_WFE_OPTION (CMDQ_WFE_UPDATE | CMDQ_WFE_WAIT | \
> + CMDQ_WFE_WAIT_VALUE)
> /** cmdq event maximum */
> #define CMDQ_MAX_EVENT 0x3ff
>
^ permalink raw reply
* Re: [PATCH] of: Fix of_empty_ranges_quirk()
From: Marek Vasut @ 2019-08-10 13:39 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree, Marek Vasut, Frank Rowand,
open list:MEDIA DRIVERS FOR RENESAS - FCP
In-Reply-To: <CAL_JsqJyYQ99ENOkNd6yzn1eYwLTGLNihFxtovSPJajtF9SVvg@mail.gmail.com>
On 8/10/19 12:34 AM, Rob Herring wrote:
> On Fri, Aug 9, 2019 at 11:33 AM <marek.vasut@gmail.com> wrote:
>>
>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>>
>> The of_empty_ranges_quirk() returns a mix of boolean and signed integer
>> types, which cannot work well.
>
> It never returns a negative. The negative is used as an uninitialized
> flag. Note quirk_state is static.
It's still mixing boolean and signed int types though, which isn't right.
>> Replace that with boolean only and fix
>> usage logic in of_translate_one() -- the check should trigger when the
>> ranges are NULL and the quirk is applicable on the hardware.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Frank Rowand <frowand.list@gmail.com>
>> Cc: linux-renesas-soc@vger.kernel.org
>> To: devicetree@vger.kernel.org
>> ---
>> drivers/of/address.c | 9 +++++----
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/of/address.c b/drivers/of/address.c
>> index b492176c0572..ae2819e148b8 100644
>> --- a/drivers/of/address.c
>> +++ b/drivers/of/address.c
>> @@ -616,7 +616,7 @@ static struct of_bus *of_match_bus(struct device_node *np)
>> return NULL;
>> }
>>
>> -static int of_empty_ranges_quirk(struct device_node *np)
>> +static bool of_empty_ranges_quirk(struct device_node *np)
>> {
>> if (IS_ENABLED(CONFIG_PPC)) {
>> /* To save cycles, we cache the result for global "Mac" setting */
>> @@ -631,7 +631,8 @@ static int of_empty_ranges_quirk(struct device_node *np)
>> quirk_state =
>> of_machine_is_compatible("Power Macintosh") ||
>> of_machine_is_compatible("MacRISC");
>> - return quirk_state;
>> + if (quirk_state > 0)
>> + return true;
>> }
>> return false;
>> }
>> @@ -662,8 +663,8 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>> * This code is only enabled on powerpc. --gcl
>> */
>> ranges = of_get_property(parent, rprop, &rlen);
>> - if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
>> - pr_debug("no ranges; cannot translate\n");
>> + if (ranges == NULL && of_empty_ranges_quirk(parent)) {
>> + pr_err("no ranges; cannot translate\n");
>
> This is wrong. If you have NULL ranges and not the quirk, then no
> ranges is an error. IOW, if you are getting an error here, you have an
> error in your DT (because I assume you are not working on a PASemi or
> Apple system).
The way I understand the code is that
if (you have no ranges in the DT) AND (the quirk is applicable) then
print the message. Which is what this patch does.
Am I missing something ?
--
Best regards,
Marek Vasut
^ permalink raw reply
* Re: [v4 2/6] media: platform: dwc: Add MIPI CSI-2 controller driver
From: Andy Shevchenko @ 2019-08-10 13:09 UTC (permalink / raw)
To: Sakari Ailus
Cc: Luis Oliveira, Mauro Carvalho Chehab, David S. Miller,
Greg Kroah-Hartman, Jonathan Cameron, Rob Herring, Nicolas Ferre,
paulmck, Mark Rutland, Kishon Vijay Abraham I, devicetree,
Linux Media Mailing List, Linux Kernel Mailing List, Joao Pinto
In-Reply-To: <20190809141000.GB864@valkosipuli.retiisi.org.uk>
On Fri, Aug 9, 2019 at 5:38 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:
> On Tue, Jun 11, 2019 at 09:20:51PM +0200, Luis Oliveira wrote:
> > Add the Synopsys MIPI CSI-2 controller driver. This
> > controller driver is divided in platform functions and core functions.
> > This way it serves as platform for future DesignWare drivers.
> > +const struct mipi_dt csi_dt[] = {
>
> Make this static or use a common prefix that somehow resembles the name
> name of the driver.
>
> > + {
> > + .hex = CSI_2_YUV420_8,
> > + .name = "YUV420_8bits",
> > + }, {
> > + .hex = CSI_2_YUV420_10,
> > + .name = "YUV420_10bits",
> > + }, {
> > + .hex = CSI_2_YUV420_8_LEG,
> > + .name = "YUV420_8bits_LEGACY",
> > + }, {
> > + .hex = CSI_2_YUV420_8_SHIFT,
> > + .name = "YUV420_8bits_SHIFT",
> > + }, {
> > + .hex = CSI_2_YUV420_10_SHIFT,
> > + .name = "YUV420_10bits_SHIFT",
> > + }, {
> > + .hex = CSI_2_YUV422_8,
> > + .name = "YUV442_8bits",
> > + }, {
> > + .hex = CSI_2_YUV422_10,
> > + .name = "YUV442_10bits",
> > + }, {
> > + .hex = CSI_2_RGB444,
> > + .name = "RGB444",
> > + }, {
> > + .hex = CSI_2_RGB555,
> > + .name = "RGB555",
> > + }, {
> > + .hex = CSI_2_RGB565,
> > + .name = "RGB565",
> > + }, {
> > + .hex = CSI_2_RGB666,
> > + .name = "RGB666",
> > + }, {
> > + .hex = CSI_2_RGB888,
> > + .name = "RGB888",
> > + }, {
> > + .hex = CSI_2_RAW6,
> > + .name = "RAW6",
> > + }, {
> > + .hex = CSI_2_RAW7,
> > + .name = "RAW7",
> > + }, {
> > + .hex = CSI_2_RAW8,
> > + .name = "RAW8",
> > + }, {
> > + .hex = CSI_2_RAW10,
> > + .name = "RAW10",
> > + }, {
> > + .hex = CSI_2_RAW12,
> > + .name = "RAW12",
> > + }, {
> > + .hex = CSI_2_RAW14,
> > + .name = "RAW14",
> > + }, {
> > + .hex = CSI_2_RAW16,
> > + .name = "RAW16",
> > + },
> > +};
One may utilize __stringify() macro and do somelike
#define CSI_FMT_DESC(fmt) \
{ .hex = CSI_2_##fmt, .name = __stringify(fmt), }
And do
CSI_FMT_DESC(RAW16),
etc.
> > + return cfg ? v4l2_subdev_get_try_format(&dev->sd,
> > + cfg,
> > + 0) : NULL;
This indentation looks ugly.
I would rather put this on one line.
> > + dev_dbg(dev->dev,
> > + "%s got v4l2_mbus_pixelcode. 0x%x\n", __func__,
> > + dev->format.code);
> > + dev_dbg(dev->dev,
> > + "%s got width. 0x%x\n", __func__,
> > + dev->format.width);
> > + dev_dbg(dev->dev,
> > + "%s got height. 0x%x\n", __func__,
> > + dev->format.height);
__func__ is usually redundant (if Dynamic Debug in use it can be
switched at run-time).
> I'd just omit these debug prints in a driver. But adding them to the
> framework might make sense. We don't have a lot of debug prints dealing
> with user parameters in there. OTOH the common test programs largely do the
> same already.
I would rather see tracepoints instead of debug prints if we are
talking about generic solution for entire framework.
>
> > + return &dev->format;
> > +}
> > + struct mipi_fmt *dev_fmt;
This is simple bad name. We have dev_fmt() macro. I would rather avoid
potential collisions.
> > + struct v4l2_mbus_framefmt *mf;
> > +
> > + mf = dw_mipi_csi_get_format(dev, cfg, fmt->which);
> > + if (!mf)
> > + return -EINVAL;
Can't you rather return an error pointer in this and similar cases?
> > + dev_vdbg(dev->dev, "%s: on=%d\n", __func__, on);
This is noise. If you would like to debug Function Tracer is a good start.
> > + of_id = of_match_node(dw_mipi_csi_of_match, dev->of_node);
> > + if (!of_id)
> > + return -EINVAL;
Is it possible to have this asserted?
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res)
> > + return -ENXIO;
Redundant. Below does the check for you.
> > +
> > + csi->base_address = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(csi->base_address)) {
> > + dev_err(dev, "Base address not set.\n");
Redundant. Above does print an error message for you.
> > + return PTR_ERR(csi->base_address);
> > + }
Moreover, use devm_platform_ioremap_resource() instead of both.
> > + csi->ctrl_irq_number = platform_get_irq(pdev, 0);
> > + if (csi->ctrl_irq_number < 0) {
> > + dev_err(dev, "irq number %d not set.\n", csi->ctrl_irq_number);
Redundant since this cycle (v5.4).
> > + ret = csi->ctrl_irq_number;
Better to do the opposite
ret = platform_get_irq();
if (ret)
goto end;
... = ret;
> > + goto end;
> > + }
> > + ret = devm_request_irq(dev, csi->ctrl_irq_number,
> > + dw_mipi_csi_irq1, IRQF_SHARED,
> > + dev_name(dev), csi);
> > + if (ret) {
> > + dev_err(dev, "irq csi %s failed\n", of_id->name);
> > +
> > + goto end;
> > + }
devm_*irq() might be a bad idea. Is it race free in your driver?
> > +static const struct of_device_id dw_mipi_csi_of_match[] = {
> > + { .compatible = "snps,dw-csi" },
> > + {},
Better without comma. Terminator may terminate even at compile time.
> > +};
> > +static ssize_t core_version_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct platform_device *pdev = to_platform_device(dev);
> > + struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> > + struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> > +
> > + char buffer[10];
> > +
> > + snprintf(buffer, 10, "v.%d.%d*\n", csi_dev->hw_version_major,
> > + csi_dev->hw_version_minor);
> > +
> > + return strlcpy(buf, buffer, PAGE_SIZE);
Oh, can't you simple without any temprorary useless buffers?
sprintf(buf, ...)?
(Yes, note _absence_ of *n* there)
> > +}
> > +static ssize_t n_lanes_store(struct device *dev, struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + int ret;
> > + unsigned long lanes;
> > +
More blank lines! We need them!
> > + struct platform_device *pdev = to_platform_device(dev);
> > + struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> > + struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> > +
> > + ret = kstrtoul(buf, 10, &lanes);
> > + if (ret < 0)
> > + return ret;
Can it return positive number?
> > + dev_info(dev, "Lanes %lu\n", lanes);
Noise.
The user gets it, why to spam kernel log???
> > + csi_dev->hw.num_lanes = lanes;
> > +
> > + return count;
> > +}
I told once, can repeat again. Synopsys perhaps needs better reviews
inside company. Each time I see the code, it repeats same mistakes
over and over. Have you, guys, do something about it?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2] dt-bindings: usb: renesas_gen3: Rename bindings documentation file to reflect IP block
From: Greg Kroah-Hartman @ 2019-08-10 11:14 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Simon Horman, Yoshihiro Shimoda, Geert Uytterhoeven,
Niklas Söderlund, Magnus Damm, Rob Herring, Mark Rutland,
USB list,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux-Renesas
In-Reply-To: <CAMuHMdUHK7Fq3m4y1rjVFxnSXH3tZyTjOzFMfVMtRtPcdKjNCw@mail.gmail.com>
On Sat, Aug 10, 2019 at 08:40:15AM +0200, Geert Uytterhoeven wrote:
> Hi Simon,
>
> On Fri, Aug 9, 2019 at 11:37 PM Simon Horman <horms+renesas@verge.net.au> wrote:
> > For consistency with the naming of (most) other documentation files for DT
> > bindings for Renesas IP blocks rename the Renesas USB3.0 peripheral
> > documentation file from renesas,usb3.txt to renesas,usb3-peri.txt
> >
> > This refines a recent rename from renesas_usb3.txt to renesas-usb3.txt.
>
> s/renesas-usb3.txt/renesas,usb3.txt/
I'll fix it up now, no need for a resend...
^ permalink raw reply
* Re: [PATCH v2] dt-bindings: usb: renesas_gen3: Rename bindings documentation file to reflect IP block
From: Sergei Shtylyov @ 2019-08-10 10:19 UTC (permalink / raw)
To: Simon Horman, Greg Kroah-Hartman
Cc: Yoshihiro Shimoda, Geert Uytterhoeven, Niklas Söderlund,
Magnus Damm, Rob Herring, Mark Rutland, linux-usb, devicetree,
linux-renesas-soc
In-Reply-To: <20190809213710.31783-1-horms+renesas@verge.net.au>
Hello!
On 10.08.2019 0:37, Simon Horman wrote:
> For consistency with the naming of (most) other documentation files for DT
> bindings for Renesas IP blocks rename the Renesas USB3.0 peripheral
> documentation file from renesas,usb3.txt to renesas,usb3-peri.txt
>
> This refines a recent rename from renesas_usb3.txt to renesas-usb3.txt.
To renesas,usb3.txt, perhaps? That's what I'm seeing as the original file
in this patch...
> The motivation is to more accurately reflect the IP block documented in
> this file.
>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> * Based on v5.3-rc1
>
> v2
> * Add review tag
> * Correct changelog
> ---
> .../devicetree/bindings/usb/{renesas,usb3.txt => renesas,usb3-peri.txt} | 0
> 1 file changed, 0 insertions(+), 0 deletions(-)
> rename Documentation/devicetree/bindings/usb/{renesas,usb3.txt => renesas,usb3-peri.txt} (100%)
>
> diff --git a/Documentation/devicetree/bindings/usb/renesas,usb3.txt b/Documentation/devicetree/bindings/usb/renesas,usb3-peri.txt
> similarity index 100%
> rename from Documentation/devicetree/bindings/usb/renesas,usb3.txt
> rename to Documentation/devicetree/bindings/usb/renesas,usb3-peri.txt
MBR, Sergei
^ permalink raw reply
* [PATCH] dt-bindings: Add vendor prefix for Inspur Corporation
From: John Wang @ 2019-08-10 9:58 UTC (permalink / raw)
To: robh+dt, mark.rutland, devicetree, linux-kernel, duanzhijia01,
linux
Signed-off-by: John Wang <wangzqbj@inspur.com>
---
Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 33a65a45e319..cc07237b8b89 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -401,6 +401,8 @@ patternProperties:
description: Innolux Corporation
"^inside-secure,.*":
description: INSIDE Secure
+ "^inspur,.*":
+ description: Inspur Corporation
"^intel,.*":
description: Intel Corporation
"^intercontrol,.*":
--
2.17.1
^ permalink raw reply related
* [PATCH v2 1/2] dt-bindings: Add ipsps1 as a trivial device
From: John Wang @ 2019-08-10 9:53 UTC (permalink / raw)
To: robh+dt, mark.rutland, trivial, linux, venture, jgebben,
anson.huang, devicetree, openbmc, duanzhijia01, mine260309
The ipsps1 is an Inspur Power System power supply unit
Signed-off-by: John Wang <wangzqbj@inspur.com>
---
v2:
- No changes.
---
Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index 747fd3f689dc..63960c7d949a 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -101,6 +101,8 @@ properties:
# Infineon TLV493D-A1B6 I2C 3D Magnetic Sensor
- infineon,tlv493d-a1b6
# Intersil ISL29028 Ambient Light and Proximity Sensor
+ - inspur,ipsps1
+ # Inspur Power System power supply unit version 1
- isil,isl29028
# Intersil ISL29030 Ambient Light and Proximity Sensor
- isil,isl29030
--
2.17.1
^ permalink raw reply related
* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
From: Linus Walleij @ 2019-08-10 8:51 UTC (permalink / raw)
To: Rob Herring
Cc: Harish Jenny K N, Bartosz Golaszewski, Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list:GPIO SUBSYSTEM, Balasubramani Vivekanandan
In-Reply-To: <CAL_JsqLp___2O-naU+2PPQy0QmJX6+aN3hByz-OB9+qFvWgN9Q@mail.gmail.com>
On Fri, Aug 9, 2019 at 4:08 PM Rob Herring <robh+dt@kernel.org> wrote:
> On Mon, Aug 5, 2019 at 5:15 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > There is some level of ambition here which is inherently a bit fuzzy
> > around the edges. ("How long is the coast of Britain?" comes to mind.)
> >
> > Surely the intention of device tree is not to recreate the schematic
> > in all detail. What we want is a model of the hardware that will
> > suffice for the operating system usecases.
> >
> > But sometimes the DTS files will become confusing: why is this
> > component using GPIO_ACTIVE_LOW when another system
> > doesn't have that flag? If there is an explicit inverter, the
> > DTS gets more readable for a human.
> >
> > But arguable that is case for adding inverters as syntactic
> > sugar in the DTS compiler instead...
>
> If you really want something more explicit, then add a new GPIO
> 'inverted' flag. Then a device can always have the same HIGH/LOW flag.
> That also solves the abstract it for userspace problem.
I think there are some intricate ontologies at work here.
Consider this example: a GPIO is controlling a chip select
regulator, say Acme Foo. The chip select
has a pin named CSN. We know from convention that the
"N" at the end of that pin name means "negative" i.e. active
low, and that is how the electronics engineers think about
that chip select line: it activates the IC when
the line goes low.
The regulator subsystem and I think all subsystems in the
Linux kernel say the consumer pin should be named and
tagged after the datsheet of the regulator.
So it has for example:
foo {
compatible = "acme,foo";
cs-gpios = <&gpio0 6 GPIO_ACTIVE_LOW>;
};
(It would be inappropriate to name it "csn-gpios" since
we have an established flag for active low. But it is another
of these syntactic choices where people likely do mistakes.)
I think it would be appropriate for the DT binding to say
that this flag must always be GPIO_ACTIVE_LOW since
the bindings are seen from the component point of view,
and thus this is always active low.
It would even be reasonable for a yaml schema to enfore
this, if it could. It is defined as active low after all.
Now if someone adds an inverter on that line between
gpio0 and Acme Foo it looks like this:
foo {
compatible = "acme,foo";
cs-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
};
And now we get cognitive dissonance or whatever I should
call it: someone reading this DTS sheet and the data
sheet for the component Acme Foo to troubleshoot
this will be confused: this component has CS active
low and still it is specified as active high? Unless they
also look at the schematic or the board and find the
inverter things are pretty muddy and they will likely curse
and solve the situation with the usual trial-and-error,
inserting some random cursewords as a comment.
With an intermediate inverter node, the cs-gpios
can go back to GPIO_ACTIVE_LOW and follow
the bindings:
inv0: inverter {
compatible = "gpio-inverter";
gpio-controller;
#gpio-cells = <1>;
inverted-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
};
foo {
compatible = "acme,foo";
cs-gpios = <&inv0 0 GPIO_ACTIVE_LOW>;
};
And now Acme Foo bindings can keep enforcing cs-gpios
to always be tagged GPIO_ACTIVE_LOW.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v4 0/7] Allwinner H6 SPDIF support
From: Clément Péron @ 2019-08-10 8:45 UTC (permalink / raw)
To: Mark Brown, Maxime Ripard
Cc: Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Linux-ALSA,
devicetree, linux-arm-kernel, Mark Rutland, Rob Herring,
Chen-Yu Tsai, linux-kernel, linux-sunxi, Jagan Teki
In-Reply-To: <20190715193842.GC4503@sirena.org.uk>
Hi,
Sorry, I just discovered that the ASoC patches have been merged into
the broonie and linus tree in 5.3.
I'm still quite new in the sending of patches to the Kernel but
souldn't be a ack or a mail sent to warn the sender when the series
are accepted?
Should 5/6/7 patches be picked by Sunxi maintainer?
Thanks,
Clément
On Mon, 15 Jul 2019 at 21:38, Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Jul 15, 2019 at 09:21:01PM +0200, Clément Péron wrote:
> > Hi,
> >
> > I'm missing ACK from ASoC Maintainers patch 2-3-4.
> >
> > It's really small paches, if you could have a look at it.
>
> Please don't send content free pings and please allow a reasonable time
> for review. People get busy, go on holiday, attend conferences and so
> on so unless there is some reason for urgency (like critical bug fixes)
> please allow at least a couple of weeks for review. If there have been
> review comments then people may be waiting for those to be addressed.
>
> Sending content free pings adds to the mail volume (if they are seen at
> all) which is often the problem and since they can't be reviewed
> directly if something has gone wrong you'll have to resend the patches
> anyway, so sending again is generally a better approach though there are
> some other maintainers who like them - if in doubt look at how patches
> for the subsystem are normally handled.
^ permalink raw reply
* [PATCH v9 21/21] iommu/mediatek: Clean up struct mtk_smi_iommu
From: Yong Wu @ 2019-08-10 7:58 UTC (permalink / raw)
To: Joerg Roedel, Matthias Brugger, Robin Murphy, Will Deacon
Cc: Rob Herring, Evan Green, Tomasz Figa, linux-mediatek,
srv_heupstream, devicetree, linux-kernel, linux-arm-kernel, iommu,
yong.wu, youlin.pei, Nicolas Boichat, anan.sun, Matthias Kaehlcke,
cui.zhang, chao.hao, ming-fan.chen
In-Reply-To: <1565423901-17008-1-git-send-email-yong.wu@mediatek.com>
Remove the "struct mtk_smi_iommu" to simplify the code since it has only
one item in it right now.
Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
drivers/iommu/mtk_iommu.c | 4 ++--
drivers/iommu/mtk_iommu.h | 6 +++---
drivers/iommu/mtk_iommu_v1.c | 4 ++--
drivers/memory/mtk-smi.c | 6 +++---
include/soc/mediatek/smi.h | 4 ----
5 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 274761c..d5b9454 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -254,7 +254,7 @@ static void mtk_iommu_config(struct mtk_iommu_data *data,
for (i = 0; i < fwspec->num_ids; ++i) {
larbid = MTK_M4U_TO_LARB(fwspec->ids[i]);
portid = MTK_M4U_TO_PORT(fwspec->ids[i]);
- larb_mmu = &data->smi_imu.larb_imu[larbid];
+ larb_mmu = &data->larb_imu[larbid];
dev_dbg(dev, "%s iommu port: %d\n",
enable ? "enable" : "disable", portid);
@@ -653,7 +653,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
of_node_put(larbnode);
return -EPROBE_DEFER;
}
- data->smi_imu.larb_imu[id].dev = &plarbdev->dev;
+ data->larb_imu[id].dev = &plarbdev->dev;
component_match_add_release(dev, &match, release_of,
compare_of, larbnode);
diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
index 56b579c..fc0f16e 100644
--- a/drivers/iommu/mtk_iommu.h
+++ b/drivers/iommu/mtk_iommu.h
@@ -56,7 +56,6 @@ struct mtk_iommu_data {
struct mtk_iommu_suspend_reg reg;
struct mtk_iommu_domain *m4u_dom;
struct iommu_group *m4u_group;
- struct mtk_smi_iommu smi_imu; /* SMI larb iommu info */
bool enable_4GB;
bool tlb_flush_active;
@@ -64,6 +63,7 @@ struct mtk_iommu_data {
const struct mtk_iommu_plat_data *plat_data;
struct list_head list;
+ struct mtk_smi_larb_iommu larb_imu[MTK_LARB_NR_MAX];
};
static inline int compare_of(struct device *dev, void *data)
@@ -80,14 +80,14 @@ static inline int mtk_iommu_bind(struct device *dev)
{
struct mtk_iommu_data *data = dev_get_drvdata(dev);
- return component_bind_all(dev, &data->smi_imu);
+ return component_bind_all(dev, &data->larb_imu);
}
static inline void mtk_iommu_unbind(struct device *dev)
{
struct mtk_iommu_data *data = dev_get_drvdata(dev);
- component_unbind_all(dev, &data->smi_imu);
+ component_unbind_all(dev, &data->larb_imu);
}
#endif
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index 3922358..860926c 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -206,7 +206,7 @@ static void mtk_iommu_config(struct mtk_iommu_data *data,
for (i = 0; i < fwspec->num_ids; ++i) {
larbid = mt2701_m4u_to_larb(fwspec->ids[i]);
portid = mt2701_m4u_to_port(fwspec->ids[i]);
- larb_mmu = &data->smi_imu.larb_imu[larbid];
+ larb_mmu = &data->larb_imu[larbid];
dev_dbg(dev, "%s iommu port: %d\n",
enable ? "enable" : "disable", portid);
@@ -610,7 +610,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
}
}
- data->smi_imu.larb_imu[larb_nr].dev = &plarbdev->dev;
+ data->larb_imu[larb_nr].dev = &plarbdev->dev;
component_match_add_release(dev, &match, release_of,
compare_of, larb_spec.np);
larb_nr++;
diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
index d6dc62f..439d7d8 100644
--- a/drivers/memory/mtk-smi.c
+++ b/drivers/memory/mtk-smi.c
@@ -143,13 +143,13 @@ void mtk_smi_larb_put(struct device *larbdev)
mtk_smi_larb_bind(struct device *dev, struct device *master, void *data)
{
struct mtk_smi_larb *larb = dev_get_drvdata(dev);
- struct mtk_smi_iommu *smi_iommu = data;
+ struct mtk_smi_larb_iommu *larb_mmu = data;
unsigned int i;
for (i = 0; i < MTK_LARB_NR_MAX; i++) {
- if (dev == smi_iommu->larb_imu[i].dev) {
+ if (dev == larb_mmu[i].dev) {
larb->larbid = i;
- larb->mmu = &smi_iommu->larb_imu[i].mmu;
+ larb->mmu = &larb_mmu[i].mmu;
return 0;
}
}
diff --git a/include/soc/mediatek/smi.h b/include/soc/mediatek/smi.h
index 6f0b00c..5a34b87 100644
--- a/include/soc/mediatek/smi.h
+++ b/include/soc/mediatek/smi.h
@@ -20,10 +20,6 @@ struct mtk_smi_larb_iommu {
unsigned int mmu;
};
-struct mtk_smi_iommu {
- struct mtk_smi_larb_iommu larb_imu[MTK_LARB_NR_MAX];
-};
-
/*
* mtk_smi_larb_get: Enable the power domain and clocks for this local arbiter.
* It also initialize some basic setting(like iommu).
--
1.9.1
^ permalink raw reply related
* [PATCH v9 20/21] memory: mtk-smi: Get rid of need_larbid
From: Yong Wu @ 2019-08-10 7:58 UTC (permalink / raw)
To: Joerg Roedel, Matthias Brugger, Robin Murphy, Will Deacon
Cc: Rob Herring, Evan Green, Tomasz Figa, linux-mediatek,
srv_heupstream, devicetree, linux-kernel, linux-arm-kernel, iommu,
yong.wu, youlin.pei, Nicolas Boichat, anan.sun, Matthias Kaehlcke,
cui.zhang, chao.hao, ming-fan.chen
In-Reply-To: <1565423901-17008-1-git-send-email-yong.wu@mediatek.com>
The "mediatek,larb-id" has already been parsed in MTK IOMMU driver.
It's no need to parse it again in SMI driver. Only clean some codes.
This patch is fit for all the current mt2701, mt2712, mt7623, mt8173
and mt8183.
After this patch, the "mediatek,larb-id" only be needed for mt2712
which have 2 M4Us. In the other SoCs, we can get the larb-id from M4U
in which the larbs in the "mediatek,larbs" always are ordered.
Correspondingly, the larb_nr in the "struct mtk_smi_iommu" could also
be deleted.
CC: Matthias Brugger <matthias.bgg@gmail.com>
Signed-off-by: Yong Wu <yong.wu@mediatek.com>
Reviewed-by: Evan Green <evgreen@chromium.org>
Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
---
drivers/iommu/mtk_iommu.c | 1 -
drivers/iommu/mtk_iommu_v1.c | 2 --
drivers/memory/mtk-smi.c | 26 ++------------------------
include/soc/mediatek/smi.h | 1 -
4 files changed, 2 insertions(+), 28 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 4f2f114..274761c 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -629,7 +629,6 @@ static int mtk_iommu_probe(struct platform_device *pdev)
"mediatek,larbs", NULL);
if (larb_nr < 0)
return larb_nr;
- data->smi_imu.larb_nr = larb_nr;
for (i = 0; i < larb_nr; i++) {
struct device_node *larbnode;
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index abeeac4..3922358 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -616,8 +616,6 @@ static int mtk_iommu_probe(struct platform_device *pdev)
larb_nr++;
}
- data->smi_imu.larb_nr = larb_nr;
-
platform_set_drvdata(pdev, data);
ret = mtk_iommu_hw_init(data);
diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
index 289e595..d6dc62f 100644
--- a/drivers/memory/mtk-smi.c
+++ b/drivers/memory/mtk-smi.c
@@ -59,7 +59,6 @@ struct mtk_smi_common_plat {
};
struct mtk_smi_larb_gen {
- bool need_larbid;
int port_in_larb[MTK_LARB_NR_MAX + 1];
void (*config_port)(struct device *);
unsigned int larb_direct_to_common_mask;
@@ -147,18 +146,9 @@ void mtk_smi_larb_put(struct device *larbdev)
struct mtk_smi_iommu *smi_iommu = data;
unsigned int i;
- if (larb->larb_gen->need_larbid) {
- larb->mmu = &smi_iommu->larb_imu[larb->larbid].mmu;
- return 0;
- }
-
- /*
- * If there is no larbid property, Loop to find the corresponding
- * iommu information.
- */
- for (i = 0; i < smi_iommu->larb_nr; i++) {
+ for (i = 0; i < MTK_LARB_NR_MAX; i++) {
if (dev == smi_iommu->larb_imu[i].dev) {
- /* The 'mmu' may be updated in iommu-attach/detach. */
+ larb->larbid = i;
larb->mmu = &smi_iommu->larb_imu[i].mmu;
return 0;
}
@@ -237,7 +227,6 @@ static void mtk_smi_larb_config_port_gen1(struct device *dev)
};
static const struct mtk_smi_larb_gen mtk_smi_larb_mt2701 = {
- .need_larbid = true,
.port_in_larb = {
LARB0_PORT_OFFSET, LARB1_PORT_OFFSET,
LARB2_PORT_OFFSET, LARB3_PORT_OFFSET
@@ -246,7 +235,6 @@ static void mtk_smi_larb_config_port_gen1(struct device *dev)
};
static const struct mtk_smi_larb_gen mtk_smi_larb_mt2712 = {
- .need_larbid = true,
.config_port = mtk_smi_larb_config_port_gen2_general,
.larb_direct_to_common_mask = BIT(8) | BIT(9), /* bdpsys */
};
@@ -285,7 +273,6 @@ static int mtk_smi_larb_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct device_node *smi_node;
struct platform_device *smi_pdev;
- int err;
larb = devm_kzalloc(dev, sizeof(*larb), GFP_KERNEL);
if (!larb)
@@ -315,15 +302,6 @@ static int mtk_smi_larb_probe(struct platform_device *pdev)
}
larb->smi.dev = dev;
- if (larb->larb_gen->need_larbid) {
- err = of_property_read_u32(dev->of_node, "mediatek,larb-id",
- &larb->larbid);
- if (err) {
- dev_err(dev, "missing larbid property\n");
- return err;
- }
- }
-
smi_node = of_parse_phandle(dev->of_node, "mediatek,smi", 0);
if (!smi_node)
return -EINVAL;
diff --git a/include/soc/mediatek/smi.h b/include/soc/mediatek/smi.h
index 79b74ce..6f0b00c 100644
--- a/include/soc/mediatek/smi.h
+++ b/include/soc/mediatek/smi.h
@@ -21,7 +21,6 @@ struct mtk_smi_larb_iommu {
};
struct mtk_smi_iommu {
- unsigned int larb_nr;
struct mtk_smi_larb_iommu larb_imu[MTK_LARB_NR_MAX];
};
--
1.9.1
^ permalink raw reply related
* [PATCH v9 19/21] iommu/mediatek: Fix VLD_PA_RNG register backup when suspend
From: Yong Wu @ 2019-08-10 7:58 UTC (permalink / raw)
To: Joerg Roedel, Matthias Brugger, Robin Murphy, Will Deacon
Cc: youlin.pei-NuS5LvNUpcJWk0Htik3J/w,
devicetree-u79uwXL29TY76Z2rM5mHXA, Nicolas Boichat,
cui.zhang-NuS5LvNUpcJWk0Htik3J/w,
srv_heupstream-NuS5LvNUpcJWk0Htik3J/w,
chao.hao-NuS5LvNUpcJWk0Htik3J/w,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Evan Green, Tomasz Figa,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Rob Herring,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
ming-fan.chen-NuS5LvNUpcJWk0Htik3J/w,
anan.sun-NuS5LvNUpcJWk0Htik3J/w, Matthias Kaehlcke,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <1565423901-17008-1-git-send-email-yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
The register VLD_PA_RNG(0x118) was forgot to backup while adding 4GB
mode support for mt2712. this patch add it.
Fixes: 30e2fccf9512 ("iommu/mediatek: Enlarge the validate PA range
for 4GB mode")
Signed-off-by: Yong Wu <yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
Reviewed-by: Evan Green <evgreen-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Reviewed-by: Matthias Brugger <matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
drivers/iommu/mtk_iommu.c | 2 ++
drivers/iommu/mtk_iommu.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index cbebe11..4f2f114 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -715,6 +715,7 @@ static int __maybe_unused mtk_iommu_suspend(struct device *dev)
reg->int_control0 = readl_relaxed(base + REG_MMU_INT_CONTROL0);
reg->int_main_control = readl_relaxed(base + REG_MMU_INT_MAIN_CONTROL);
reg->ivrp_paddr = readl_relaxed(base + REG_MMU_IVRP_PADDR);
+ reg->vld_pa_rng = readl_relaxed(base + REG_MMU_VLD_PA_RNG);
clk_disable_unprepare(data->bclk);
return 0;
}
@@ -739,6 +740,7 @@ static int __maybe_unused mtk_iommu_resume(struct device *dev)
writel_relaxed(reg->int_control0, base + REG_MMU_INT_CONTROL0);
writel_relaxed(reg->int_main_control, base + REG_MMU_INT_MAIN_CONTROL);
writel_relaxed(reg->ivrp_paddr, base + REG_MMU_IVRP_PADDR);
+ writel_relaxed(reg->vld_pa_rng, base + REG_MMU_VLD_PA_RNG);
if (m4u_dom)
writel(m4u_dom->cfg.arm_v7s_cfg.ttbr[0] & MMU_PT_ADDR_MASK,
base + REG_MMU_PT_BASE_ADDR);
diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
index 6b1f833..56b579c 100644
--- a/drivers/iommu/mtk_iommu.h
+++ b/drivers/iommu/mtk_iommu.h
@@ -24,6 +24,7 @@ struct mtk_iommu_suspend_reg {
u32 int_control0;
u32 int_main_control;
u32 ivrp_paddr;
+ u32 vld_pa_rng;
};
enum mtk_iommu_plat {
--
1.9.1
^ permalink raw reply related
* [PATCH v9 18/21] memory: mtk-smi: Add bus_sel for mt8183
From: Yong Wu @ 2019-08-10 7:58 UTC (permalink / raw)
To: Joerg Roedel, Matthias Brugger, Robin Murphy, Will Deacon
Cc: youlin.pei-NuS5LvNUpcJWk0Htik3J/w,
devicetree-u79uwXL29TY76Z2rM5mHXA, Nicolas Boichat,
cui.zhang-NuS5LvNUpcJWk0Htik3J/w,
srv_heupstream-NuS5LvNUpcJWk0Htik3J/w,
chao.hao-NuS5LvNUpcJWk0Htik3J/w,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Evan Green, Tomasz Figa,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Rob Herring,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
ming-fan.chen-NuS5LvNUpcJWk0Htik3J/w,
anan.sun-NuS5LvNUpcJWk0Htik3J/w, Matthias Kaehlcke,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <1565423901-17008-1-git-send-email-yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
There are 2 mmu cells in a M4U HW. we could adjust some larbs entering
mmu0 or mmu1 to balance the bandwidth via the smi-common register
SMI_BUS_SEL(0x220)(Each larb occupy 2 bits).
In mt8183, For better performance, we switch larb1/2/5/7 to enter
mmu1 while the others still keep enter mmu0.
In mt8173 and mt2712, we don't get the performance issue,
Keep its default value(0x0), that means all the larbs enter mmu0.
Note: smi gen1(mt2701/mt7623) don't have this bus_sel.
And, the base of smi-common is completely different with smi_ao_base
of gen1, thus I add new variable for that.
CC: Matthias Brugger <matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Yong Wu <yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
Reviewed-by: Evan Green <evgreen-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Reviewed-by: Matthias Brugger <matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
drivers/memory/mtk-smi.c | 24 ++++++++++++++++++++++--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
index 2bb55b86..289e595 100644
--- a/drivers/memory/mtk-smi.c
+++ b/drivers/memory/mtk-smi.c
@@ -41,6 +41,12 @@
#define SMI_LARB_NONSEC_CON(id) (0x380 + ((id) * 4))
#define F_MMU_EN BIT(0)
+/* SMI COMMON */
+#define SMI_BUS_SEL 0x220
+#define SMI_BUS_LARB_SHIFT(larbid) ((larbid) << 1)
+/* All are MMU0 defaultly. Only specialize mmu1 here. */
+#define F_MMU1_LARB(larbid) (0x1 << SMI_BUS_LARB_SHIFT(larbid))
+
enum mtk_smi_gen {
MTK_SMI_GEN1,
MTK_SMI_GEN2
@@ -49,6 +55,7 @@ enum mtk_smi_gen {
struct mtk_smi_common_plat {
enum mtk_smi_gen gen;
bool has_gals;
+ u32 bus_sel; /* Balance some larbs to enter mmu0 or mmu1 */
};
struct mtk_smi_larb_gen {
@@ -64,8 +71,10 @@ struct mtk_smi {
struct clk *clk_apb, *clk_smi;
struct clk *clk_gals0, *clk_gals1;
struct clk *clk_async; /*only needed by mt2701*/
- void __iomem *smi_ao_base;
-
+ union {
+ void __iomem *smi_ao_base; /* only for gen1 */
+ void __iomem *base; /* only for gen2 */
+ };
const struct mtk_smi_common_plat *plat;
};
@@ -402,6 +411,8 @@ static int __maybe_unused mtk_smi_larb_suspend(struct device *dev)
static const struct mtk_smi_common_plat mtk_smi_common_mt8183 = {
.gen = MTK_SMI_GEN2,
.has_gals = true,
+ .bus_sel = F_MMU1_LARB(1) | F_MMU1_LARB(2) | F_MMU1_LARB(5) |
+ F_MMU1_LARB(7),
};
static const struct of_device_id mtk_smi_common_of_ids[] = {
@@ -474,6 +485,11 @@ static int mtk_smi_common_probe(struct platform_device *pdev)
ret = clk_prepare_enable(common->clk_async);
if (ret)
return ret;
+ } else {
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ common->base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(common->base))
+ return PTR_ERR(common->base);
}
pm_runtime_enable(dev);
platform_set_drvdata(pdev, common);
@@ -489,6 +505,7 @@ static int mtk_smi_common_remove(struct platform_device *pdev)
static int __maybe_unused mtk_smi_common_resume(struct device *dev)
{
struct mtk_smi *common = dev_get_drvdata(dev);
+ u32 bus_sel = common->plat->bus_sel;
int ret;
ret = mtk_smi_clk_enable(common);
@@ -496,6 +513,9 @@ static int __maybe_unused mtk_smi_common_resume(struct device *dev)
dev_err(common->dev, "Failed to enable clock(%d).\n", ret);
return ret;
}
+
+ if (common->plat->gen == MTK_SMI_GEN2 && bus_sel)
+ writel(bus_sel, common->base + SMI_BUS_SEL);
return 0;
}
--
1.9.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox