* [Patch v2 0/2] memory: tegra: Skip restricted register access from Guest
From: Sumit Gupta @ 2024-04-02 13:26 UTC (permalink / raw)
To: krzysztof.kozlowski, robh, conor+dt, maz, mark.rutland, treding,
jonathanh
Cc: devicetree, linux-kernel, linux-tegra, amhetre, bbasu, sumitg
MC SID and Broadbast channel register access is restricted for Guest VM.
But the Tegra MC driver is considering both these regions as mandatory
and causes error on access. Change that to consider both the regions as
optional in SoC's from Tegra186 onwards. Then skip access to the
restricted registers from Guest if its region is not present in Guest DT
and hence not mapped.
Previously, the solution in [1] was not accepted. Now, handled the
problem with this alternate solution.
---
v1[1] -> v2:
- consider broadcast channel registers also as restricted along with sid.
- make sid and broadcast regions optional and access if present in dt.
- update the yaml file.
Sumit Gupta (2):
dt-bindings: make sid and broadcast reg optional
memory: tegra: make sid and broadcast regions optional
.../nvidia,tegra186-mc.yaml | 18 +++++++--------
drivers/memory/tegra/mc.c | 9 +++++++-
drivers/memory/tegra/mc.h | 18 +++++++--------
drivers/memory/tegra/tegra186.c | 22 ++++++++++---------
4 files changed, 38 insertions(+), 29 deletions(-)
[1] https://lore.kernel.org/lkml/20240206114852.8472-1-sumitg@nvidia.com/
--
2.17.1
^ permalink raw reply
* [Patch v2 1/2] dt-bindings: make sid and broadcast reg optional
From: Sumit Gupta @ 2024-04-02 13:26 UTC (permalink / raw)
To: krzysztof.kozlowski, robh, conor+dt, maz, mark.rutland, treding,
jonathanh
Cc: devicetree, linux-kernel, linux-tegra, amhetre, bbasu, sumitg
In-Reply-To: <20240402132626.24693-1-sumitg@nvidia.com>
MC SID and Broadbast channel register access is restricted for Guest VM.
Make both the regions as optional for SoC's from Tegra186 onwards.
Tegra MC driver will skip access to the restricted registers from Guest
if the respective regions are not present in the memory-controller node
of Guest DT.
Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
---
.../memory-controllers/nvidia,tegra186-mc.yaml | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
index 935d63d181d9..c52c259f7ec5 100644
--- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
@@ -146,17 +146,17 @@ allOf:
then:
properties:
reg:
- maxItems: 6
+ maxItems: 4
description: 5 memory controller channels and 1 for stream-id registers
reg-names:
items:
- - const: sid
- - const: broadcast
- const: ch0
- const: ch1
- const: ch2
- const: ch3
+ - const: sid
+ - const: broadcast
- if:
properties:
@@ -165,13 +165,11 @@ allOf:
then:
properties:
reg:
- minItems: 18
+ minItems: 16
description: 17 memory controller channels and 1 for stream-id registers
reg-names:
items:
- - const: sid
- - const: broadcast
- const: ch0
- const: ch1
- const: ch2
@@ -188,6 +186,8 @@ allOf:
- const: ch13
- const: ch14
- const: ch15
+ - const: sid
+ - const: broadcast
- if:
properties:
@@ -196,13 +196,11 @@ allOf:
then:
properties:
reg:
- minItems: 18
+ minItems: 16
description: 17 memory controller channels and 1 for stream-id registers
reg-names:
items:
- - const: sid
- - const: broadcast
- const: ch0
- const: ch1
- const: ch2
@@ -219,6 +217,8 @@ allOf:
- const: ch13
- const: ch14
- const: ch15
+ - const: sid
+ - const: broadcast
additionalProperties: false
--
2.17.1
^ permalink raw reply related
* [Patch v2 2/2] memory: tegra: make sid and broadcast regions optional
From: Sumit Gupta @ 2024-04-02 13:26 UTC (permalink / raw)
To: krzysztof.kozlowski, robh, conor+dt, maz, mark.rutland, treding,
jonathanh
Cc: devicetree, linux-kernel, linux-tegra, amhetre, bbasu, sumitg
In-Reply-To: <20240402132626.24693-1-sumitg@nvidia.com>
MC SID and Broadbast channel register access is restricted for Guest VM.
In Tegra MC driver, consider both the regions as optional and skip
access to restricted registers from Guest if a region is not present
in Guest DT.
Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
---
drivers/memory/tegra/mc.c | 9 ++++++++-
drivers/memory/tegra/mc.h | 18 +++++++++---------
drivers/memory/tegra/tegra186.c | 22 ++++++++++++----------
3 files changed, 29 insertions(+), 20 deletions(-)
diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
index 224b488794e5..d819dab1b223 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -899,6 +899,7 @@ static void tegra_mc_num_channel_enabled(struct tegra_mc *mc)
static int tegra_mc_probe(struct platform_device *pdev)
{
+ struct resource *res;
struct tegra_mc *mc;
u64 mask;
int err;
@@ -923,7 +924,13 @@ static int tegra_mc_probe(struct platform_device *pdev)
/* length of MC tick in nanoseconds */
mc->tick = 30;
- mc->regs = devm_platform_ioremap_resource(pdev, 0);
+ if (mc->soc->num_channels) {
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sid");
+ if (res)
+ mc->regs = devm_ioremap_resource(&pdev->dev, res);
+ } else {
+ mc->regs = devm_platform_ioremap_resource(pdev, 0);
+ }
if (IS_ERR(mc->regs))
return PTR_ERR(mc->regs);
diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
index c3f6655bec60..5cdb9451f364 100644
--- a/drivers/memory/tegra/mc.h
+++ b/drivers/memory/tegra/mc.h
@@ -112,11 +112,11 @@ icc_provider_to_tegra_mc(struct icc_provider *provider)
static inline u32 mc_ch_readl(const struct tegra_mc *mc, int ch,
unsigned long offset)
{
- if (!mc->bcast_ch_regs)
- return 0;
-
- if (ch == MC_BROADCAST_CHANNEL)
+ if (ch == MC_BROADCAST_CHANNEL) {
+ if (!mc->bcast_ch_regs)
+ return 0;
return readl_relaxed(mc->bcast_ch_regs + offset);
+ }
return readl_relaxed(mc->ch_regs[ch] + offset);
}
@@ -124,13 +124,13 @@ static inline u32 mc_ch_readl(const struct tegra_mc *mc, int ch,
static inline void mc_ch_writel(const struct tegra_mc *mc, int ch,
u32 value, unsigned long offset)
{
- if (!mc->bcast_ch_regs)
- return;
-
- if (ch == MC_BROADCAST_CHANNEL)
+ if (ch == MC_BROADCAST_CHANNEL) {
+ if (!mc->bcast_ch_regs)
+ return;
writel_relaxed(value, mc->bcast_ch_regs + offset);
- else
+ } else {
writel_relaxed(value, mc->ch_regs[ch] + offset);
+ }
}
static inline u32 mc_readl(const struct tegra_mc *mc, unsigned long offset)
diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
index 1b3183951bfe..7b5e9bd13ffd 100644
--- a/drivers/memory/tegra/tegra186.c
+++ b/drivers/memory/tegra/tegra186.c
@@ -26,20 +26,16 @@
static int tegra186_mc_probe(struct tegra_mc *mc)
{
struct platform_device *pdev = to_platform_device(mc->dev);
+ struct resource *res;
unsigned int i;
char name[8];
int err;
- mc->bcast_ch_regs = devm_platform_ioremap_resource_byname(pdev, "broadcast");
- if (IS_ERR(mc->bcast_ch_regs)) {
- if (PTR_ERR(mc->bcast_ch_regs) == -EINVAL) {
- dev_warn(&pdev->dev,
- "Broadcast channel is missing, please update your device-tree\n");
- mc->bcast_ch_regs = NULL;
- goto populate;
- }
-
- return PTR_ERR(mc->bcast_ch_regs);
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "broadcast");
+ if (res) {
+ mc->bcast_ch_regs = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(mc->bcast_ch_regs))
+ return PTR_ERR(mc->bcast_ch_regs);
}
mc->ch_regs = devm_kcalloc(mc->dev, mc->soc->num_channels, sizeof(*mc->ch_regs),
@@ -121,6 +117,9 @@ static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev)
if (!tegra_dev_iommu_get_stream_id(dev, &sid))
return 0;
+ if (!mc->regs)
+ return 0;
+
while (!of_parse_phandle_with_args(dev->of_node, "interconnects", "#interconnect-cells",
index, &args)) {
if (args.np == mc->dev->of_node && args.args_count != 0) {
@@ -146,6 +145,9 @@ static int tegra186_mc_resume(struct tegra_mc *mc)
#if IS_ENABLED(CONFIG_IOMMU_API)
unsigned int i;
+ if (!mc->regs)
+ return 0;
+
for (i = 0; i < mc->soc->num_clients; i++) {
const struct tegra_mc_client *client = &mc->soc->clients[i];
--
2.17.1
^ permalink raw reply related
* RE: [PATCH v7 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support
From: Peng Fan @ 2024-04-02 13:27 UTC (permalink / raw)
To: Andy Shevchenko, Peng Fan (OSS)
Cc: Sudeep Holla, Cristian Marussi, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Linus Walleij, Dan Carpenter,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-gpio@vger.kernel.org, Oleksii Moisieiev
In-Reply-To: <ZgwEtxj-qi6uy_m2@smile.fi.intel.com>
Hi Andy
> Subject: Re: [PATCH v7 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> protocol basic support
>
> On Tue, Apr 02, 2024 at 10:22:23AM +0800, Peng Fan (OSS) wrote:
>
> ...
>
> > +#include <linux/module.h>
> > +#include <linux/scmi_protocol.h>
> > +#include <linux/slab.h>
>
> Please, follow IWYU principle, a lot of headers are missed.
ok. I will try to figure out. BTW, is there an easy way to filter
out what is missed?
>
> > +#include "common.h"
> > +#include "protocols.h"
>
> ...
>
> > + ret = scmi_pinctrl_get_pin_info(ph, selector,
> > + &pi->pins[selector]);
>
> It's netter as a single line.
I try to follow 80 max chars per SCMI coding style. If Sudeep and Cristian
is ok, I could use a single line.
>
> > + if (ret)
> > + return ret;
> > + }
>
> ...
>
> > +static int scmi_pinctrl_protocol_init(const struct
> > +scmi_protocol_handle *ph) {
> > + int ret;
> > + u32 version;
> > + struct scmi_pinctrl_info *pinfo;
> > +
> > + ret = ph->xops->version_get(ph, &version);
> > + if (ret)
> > + return ret;
> > +
> > + dev_dbg(ph->dev, "Pinctrl Version %d.%d\n",
> > + PROTOCOL_REV_MAJOR(version),
> PROTOCOL_REV_MINOR(version));
> > +
> > + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
> > + if (!pinfo)
> > + return -ENOMEM;
> > +
> > + ret = scmi_pinctrl_attributes_get(ph, pinfo);
> > + if (ret)
> > + return ret;
> > +
> > + pinfo->pins = devm_kcalloc(ph->dev, pinfo->nr_pins,
> > + sizeof(*pinfo->pins), GFP_KERNEL);
> > + if (!pinfo->pins)
> > + return -ENOMEM;
> > +
> > + pinfo->groups = devm_kcalloc(ph->dev, pinfo->nr_groups,
> > + sizeof(*pinfo->groups), GFP_KERNEL);
> > + if (!pinfo->groups)
> > + return -ENOMEM;
> > +
> > + pinfo->functions = devm_kcalloc(ph->dev, pinfo->nr_functions,
> > + sizeof(*pinfo->functions),
> GFP_KERNEL);
> > + if (!pinfo->functions)
> > + return -ENOMEM;
> > +
> > + pinfo->version = version;
> > +
> > + return ph->set_priv(ph, pinfo, version);
>
> Same comments as per previous version. devm_ here is simply wrong.
> It breaks the order of freeing resources.
>
> I.o.w. I see *no guarantee* that these init-deinit functions will be properly
> called from the respective probe-remove. Moreover the latter one may also
> have its own devm allocations (which are rightfully placed) and you get
> completely out of control the resource management.
I see an old thread.
https://lore.kernel.org/linux-arm-kernel/ZJ78hBcjAhiU+ZBO@e120937-lin/#t
The free in deinit is not to free the ones alloced in init, it is to free the ones
alloced such as in scmi_pinctrl_get_function_info
Thanks,
Peng.
>
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
^ permalink raw reply
* Re: [PATCH net-next v6 14/17] dt-bindings: net: pse-pd: Add bindings for PD692x0 PSE controller
From: Rob Herring @ 2024-04-02 13:28 UTC (permalink / raw)
To: Kory Maincent
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jonathan Corbet, Luis Chamberlain, Russ Weight,
Greg Kroah-Hartman, Rafael J. Wysocki, Krzysztof Kozlowski,
Conor Dooley, Oleksij Rempel, Mark Brown, Frank Rowand,
Andrew Lunn, Heiner Kallweit, Russell King, Thomas Petazzoni,
netdev, linux-kernel, linux-doc, devicetree, Dent Project
In-Reply-To: <20240326-feature_poe-v6-14-c1011b6ea1cb@bootlin.com>
On Tue, Mar 26, 2024 at 03:04:51PM +0100, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
>
> Add the PD692x0 I2C Power Sourcing Equipment controller device tree
> bindings documentation.
>
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
>
> Changes in v2:
> - Enhance ports-matrix description.
> - Replace additionalProperties by unevaluatedProperties.
> - Drop i2c suffix.
>
> Changes in v3:
> - Remove ports-matrix parameter.
> - Add description of all physical ports and managers.
> - Add pse_pis subnode moving to the API of pse-controller binding.
> - Remove the MAINTAINERS section for this driver as I will be maintaining
> all pse-pd subsystem.
>
> Changes in v5:
> - Remove defs used only once.
> - Replace underscore by dash.
> - Add description.
> ---
> .../bindings/net/pse-pd/microchip,pd692x0.yaml | 158 +++++++++++++++++++++
> 1 file changed, 158 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/pse-pd/microchip,pd692x0.yaml b/Documentation/devicetree/bindings/net/pse-pd/microchip,pd692x0.yaml
> new file mode 100644
> index 000000000000..62ea4363cba3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/pse-pd/microchip,pd692x0.yaml
> @@ -0,0 +1,158 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/pse-pd/microchip,pd692x0.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip PD692x0 Power Sourcing Equipment controller
> +
> +maintainers:
> + - Kory Maincent <kory.maincent@bootlin.com>
> +
> +allOf:
> + - $ref: pse-controller.yaml#
> +
> +properties:
> + compatible:
> + enum:
> + - microchip,pd69200
> + - microchip,pd69210
> + - microchip,pd69220
> +
> + reg:
> + maxItems: 1
> +
> + managers:
> + type: object
> + description:
> + List of the PD69208T4/PD69204T4/PD69208M PSE managers. Each manager
> + have 4 or 8 physical ports according to the chip version. No need to
> + specify the SPI chip select as it is automatically detected by the
> + PD692x0 PSE controller. The PSE managers have to be described from
> + the lowest chip select to the greatest one, which is the detection
> + behavior of the PD692x0 PSE controller. The PD692x0 support up to
> + 12 PSE managers which can expose up to 96 physical ports. All
> + physical ports available on a manager have to be described in the
> + incremental order even if they are not used.
> +
> + properties:
> + "#address-cells":
> + const: 1
> +
> + "#size-cells":
> + const: 0
> +
> + required:
> + - "#address-cells"
> + - "#size-cells"
> +
> + patternProperties:
> + "^manager@0[0-9]|1[0-2]$":
Unit-addresses are typically in hex.
Is 'manager' something specific to this device or should be common?
> + $ref: /schemas/graph.yaml#/properties/ports
This is not using the graph binding. Furthermore, I don't want to see
new cases of 'port' node names which are not graph nodes. We have it
already with ethernet switches, but 'ethernet-port' is preferred over
'port'.
Why is this one 'managers' and the other device binding 'channels'?
> + description:
> + PD69208T4/PD69204T4/PD69208M PSE manager exposing 4 or 8 physical
> + ports.
> +
> + properties:
> + reg:
> + description:
> + Incremental index of the PSE manager starting from 0, ranging
> + from lowest to highest chip select, up to 12.
> + maxItems: 1
> +
> + patternProperties:
> + '^port@[0-7]$':
> + type: object
> + required:
> + - reg
Any property you want is allowed in this node. You are missing
'additionalProperties'.
> +
> + required:
> + - reg
> +
> +required:
> + - compatible
> + - reg
> + - pse-pis
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ethernet-pse@3c {
> + compatible = "microchip,pd69200";
> + reg = <0x3c>;
> +
> + managers {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + manager@0 {
> + reg = <0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + phys0: port@0 {
> + reg = <0>;
> + };
> +
> + phys1: port@1 {
> + reg = <1>;
> + };
> +
> + phys2: port@2 {
> + reg = <2>;
> + };
> +
> + phys3: port@3 {
> + reg = <3>;
> + };
> + };
> +
> + manager@1 {
> + reg = <1>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + phys4: port@0 {
> + reg = <0>;
> + };
> +
> + phys5: port@1 {
> + reg = <1>;
> + };
> +
> + phys6: port@2 {
> + reg = <2>;
> + };
> +
> + phys7: port@3 {
> + reg = <3>;
> + };
> + };
> + };
> +
> + pse-pis {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + pse_pi0: pse-pi@0 {
> + reg = <0>;
> + #pse-cells = <0>;
> + pairset-names = "alternative-a", "alternative-b";
> + pairsets = <&phys0>, <&phys1>;
It is very strange that you are describing the connections within a
device.
> + polarity-supported = "MDI", "S";
> + };
> + pse_pi1: pse-pi@1 {
> + reg = <1>;
> + #pse-cells = <0>;
> + pairset-names = "alternative-a";
> + pairsets = <&phys2>;
> + polarity-supported = "MDI";
> + };
> + };
> + };
> + };
>
> --
> 2.25.1
>
^ permalink raw reply
* Re: [PATCH net-next 1/3] dt-bindings: net: renesas,rzn1-gmac: Document RZ/N1 GMAC support
From: Geert Uytterhoeven @ 2024-04-02 13:43 UTC (permalink / raw)
To: Romain Gantois
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Magnus Damm, Alexandre Torgue, Jose Abreu,
Maxime Coquelin, Russell King, Clément Léger,
Thomas Petazzoni, netdev, devicetree, linux-kernel,
linux-renesas-soc, linux-stm32, linux-arm-kernel
In-Reply-To: <20240402-rzn1-gmac1-v1-1-5be2b2894d8c@bootlin.com>
Hi Romain,
On Tue, Apr 2, 2024 at 2:36 PM Romain Gantois
<romain.gantois@bootlin.com> wrote:
> From: Clément Léger <clement.leger@bootlin.com>
>
> The RZ/N1 series of MPUs feature up to two Gigabit Ethernet controllers.
> These controllers are based on Synopsys IPs. They can be connected to
> RZ/N1 RGMII/RMII converters.
>
> Add a binding that describes these GMAC devices.
>
> Signed-off-by: "Clément Léger" <clement.leger@bootlin.com>
> [rgantois: commit log]
> Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
Thanks for your patch!
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/renesas,rzn1-gmac.yaml
> +examples:
> + - |
> + #include <dt-bindings/clock/r9a06g032-sysctrl.h>
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> + ethernet@44000000 {
> + compatible = "renesas,r9a06g032-gmac", "renesas,rzn1-gmac", "snps,dwmac";
> + reg = <0x44000000 0x2000>;
> + interrupt-parent = <&gic>;
There is no need to use interrupt-parent in examples.
> + interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "macirq", "eth_wake_irq", "eth_lpi";
> + clock-names = "stmmaceth";
> + clocks = <&sysctrl R9A06G032_HCLK_GMAC0>;
If you want this to be a real example, you should add power-domains.
> + snps,multicast-filter-bins = <256>;
> + snps,perfect-filter-entries = <128>;
> + tx-fifo-depth = <2048>;
> + rx-fifo-depth = <4096>;
> + pcs-handle = <&mii_conv1>;
> + phy-mode = "mii";
> + };
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 1/2] dt-bindings: arm: ti: Add BeagleY-AI
From: Rob Herring @ 2024-04-02 13:43 UTC (permalink / raw)
To: Robert Nelson
Cc: Jason Kridner, linux-arm-kernel, Nishanth Menon, linux-kernel,
Jared McArthur, devicetree, Deepak Khatri
In-Reply-To: <20240328191205.82295-1-robertcnelson@gmail.com>
On Thu, 28 Mar 2024 14:12:04 -0500, Robert Nelson wrote:
> This board is based on ti,j722s
>
> https://beagley-ai.org/
> https://openbeagle.org/beagley-ai/beagley-ai
>
> Signed-off-by: Robert Nelson <robertcnelson@gmail.com>
> CC: Rob Herring <robh@kernel.org>
> CC: Nishanth Menon <nm@ti.com>
> CC: Jared McArthur <j-mcarthur@ti.com>
> CC: Jason Kridner <jkridner@beagleboard.org>
> CC: Deepak Khatri <lorforlinux@beagleboard.org>
> ---
> Documentation/devicetree/bindings/arm/ti/k3.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH net-next 2/3] net: stmmac: add support for RZ/N1 GMAC
From: Russell King (Oracle) @ 2024-04-02 13:49 UTC (permalink / raw)
To: Romain Gantois
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Magnus Damm, Alexandre Torgue, Jose Abreu,
Maxime Coquelin, Clément Léger, Thomas Petazzoni,
netdev, devicetree, linux-kernel, linux-renesas-soc, linux-stm32,
linux-arm-kernel
In-Reply-To: <20240402-rzn1-gmac1-v1-2-5be2b2894d8c@bootlin.com>
On Tue, Apr 02, 2024 at 02:37:01PM +0200, Romain Gantois wrote:
> + ret = stmmac_dvr_probe(dev, plat_dat, &stmmac_res);
> + if (ret)
> + return ret;
> +
> + ndev = platform_get_drvdata(pdev);
> + priv = netdev_priv(ndev);
> +
> + pcs_node = of_parse_phandle(np, "pcs-handle", 0);
> + if (pcs_node) {
> + pcs = miic_create(dev, pcs_node);
> + of_node_put(pcs_node);
> + if (IS_ERR(pcs))
> + return PTR_ERR(pcs);
> +
> + priv->hw->phylink_pcs = pcs;
> + }
I'm afraid that this fails at one of the most basic principles of kernel
multi-threaded programming. stmmac_dvr_probe() as part of its work calls
register_netdev() which publishes to userspace the network device.
Everything that is required must be setup _prior_ to publication to
userspace to avoid races, because as soon as the network device is
published, userspace can decide to bring that interface up. If one
hasn't finished the initialisation, the interface can be brought up
before that initialisation is complete.
I don't see anything obvious in the stmmac data structures that would
allow you to hook in at an appropriate point before the
register_netdev() but after the netdev has been created. The
priv->hw data structure is created by stmmac_hwif_init()
I see that drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c is also
guilty of this as well, and should be fixed. It's even worse because it
does a truck load of stuff after stmmac_dvr_probe() which it most
definitely should not be doing.
I definitely get the feeling that the structure of the stmmac driver
is really getting out of hand, and is making stuff harder for people,
and it's not improving over time - in fact, it's getting worse. It
needs a *lot* of work to bring it back to a sane model.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply
* Re: [PATCH 1/3] dt-bindings: leds: add LED_FUNCTION_FNLOCK
From: Krzysztof Kozlowski @ 2024-04-02 13:55 UTC (permalink / raw)
To: Gergo Koteles, Ike Panhc, Hans de Goede, Ilpo Järvinen,
Pavel Machek, Lee Jones, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: platform-driver-x86, linux-kernel, linux-leds, devicetree
In-Reply-To: <8ac95e85a53dc0b8cce1e27fc1cab6d19221543b.1712063200.git.soyer@irl.hu>
On 02/04/2024 15:21, Gergo Koteles wrote:
> Newer laptops have FnLock LED.
>
> Add a define for this very common function.
>
> Signed-off-by: Gergo Koteles <soyer@irl.hu>
> ---
> include/dt-bindings/leds/common.h | 1 +
Do we really need to define all these possible LED functions? Please
link to DTS user for this.
Best regards,
Krzysztof
^ permalink raw reply
* RE: [PATCH v7 4/4] pinctrl: Implementation of the generic scmi-pinctrl driver
From: Peng Fan @ 2024-04-02 13:59 UTC (permalink / raw)
To: Andy Shevchenko, Peng Fan (OSS)
Cc: Sudeep Holla, Cristian Marussi, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Linus Walleij, Dan Carpenter,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-gpio@vger.kernel.org, Oleksii Moisieiev
In-Reply-To: <ZgwGpZ6S13vjk8jh@smile.fi.intel.com>
> Subject: Re: [PATCH v7 4/4] pinctrl: Implementation of the generic scmi-
> pinctrl driver
>
> On Tue, Apr 02, 2024 at 10:22:24AM +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.
>
> ...
>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/module.h>
> > +#include <linux/seq_file.h>
> > +#include <linux/scmi_protocol.h>
> > +#include <linux/slab.h>
>
> Missing headers.
Not sure there is an easy way to filter out what is missed.
>
> ...
>
> > + *p_groups = (const char * const *)func->groups;
>
> Is this casting needed?
This is no needed.
>
> ...
>
> > +static int pinctrl_scmi_pinconf_get(struct pinctrl_dev *pctldev,
> > + unsigned int _pin, unsigned long *config)
>
> Why underscored parameter name?
Underscore could be dropped.
>
> ...
>
> > +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;
> > +
> > + 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.
> > + */
>
> This is fragile, but at least it is documented.
>
> > + 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;
> > + ret = pinctrl_ops->name_get(pmx->ph, i, PIN_TYPE,
> &pins[i].name);
> > + if (ret)
>
> How does the cleanup work for the previously assigned pin names? Is it
> needed?
No need. The "name" memory region is allocated in firmware pinctrl
Protocol init phase.
> Maybe a comment?
ok. As below.
/*
* The region for name is handled by the scmi firmware driver,
* no need free here
*/
>
> > + 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;
> > +}
>
> ...
>
> > +static const struct scmi_device_id scmi_id_table[] = {
> > + { SCMI_PROTOCOL_PINCTRL, "pinctrl" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(scmi, scmi_id_table);
>
> Move this closer to the user.
ok. Fix in v8.
Thanks,
Peng.
>
> --
> With Best Regards,
> Andy Shevchenko
>
^ permalink raw reply
* Re: [PATCH 6/6] iio: adc: ad7173: Add support for AD411x devices
From: David Lechner @ 2024-04-02 14:00 UTC (permalink / raw)
To: dumitru.ceclan
Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-iio,
devicetree, linux-kernel, Dumitru Ceclan
In-Reply-To: <CAMknhBFdtv84E_S4wa4UW0pO2yiUEk9=jn=_i4F=b8VHdR6v+w@mail.gmail.com>
On Mon, Apr 1, 2024 at 2:45 PM David Lechner <dlechner@baylibre.com> wrote:
>
> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:
> >
> > From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> >
> > Add support for AD4111/AD4112/AD4114/AD4115/AD4116.
> >
...
> > @@ -1028,15 +1204,22 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
> > *chan = ad7173_channel_template;
> > chan->address = chan_index;
> > chan->scan_index = chan_index;
> > - chan->channel = ain[0];
> > - chan->channel2 = ain[1];
> > - chan->differential = true;
> >
> > - chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
> > + if (reg >= AD4111_CURRENT_CHAN_CUTOFF) {
> > + chan->type = IIO_CURRENT;
> > + chan->channel = ain[0];
> > + chan_st_priv->ain = ad4111_current_channel_config[ain[0]];
> > + } else {
> > + chan->channel = ain[0];
> > + chan->channel2 = ain[1];
> > + chan->differential = true;
>
> Expecting chan->differential = false when ADCIN15 is configured for
> pseudo-differential inputs.
>
> Also, perhaps missed in previous reviews, I would expect
> chan->differential = false when channels are used as single-ended.
>
After sleeping on it, I came to the concision that these parts are
probably too complex to try to worry about differential vs.
pseudo-differential/single-ended (what the datasheet calls
single-ended is really pseudo-differential).
So I take back my comments about expecting differential = false in those cases.
^ permalink raw reply
* Re: [PATCH RFT 01/10] arm64: dts: microchip: sparx5: fix mdio reg
From: Steen Hegelund @ 2024-04-02 14:00 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Nicolas Ferre, Claudiu Beznea, Rob Herring, Krzysztof Kozlowski,
Lars Povlsen, Daniel Machon, UNGLinuxDriver, David S. Miller,
Bjarni Jonasson, linux-arm-kernel, devicetree, linux-kernel,
Conor Dooley
In-Reply-To: <20240401153740.123978-1-krzk@kernel.org>
Hi Krzysztof,
On Mon, 2024-04-01 at 17:37 +0200, Krzysztof Kozlowski wrote:
> [Some people who received this message don't often get email from
> krzk@kernel.org. Learn why this is important at
> https://aka.ms/LearnAboutSenderIdentification ]
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> Correct the reg address of mdio node to match unit address. Assume
> the
> reg is not correct and unit address was correct, because there is
> alerady node using the existing reg 0x110102d4.
>
> sparx5.dtsi:443.25-451.5: Warning (simple_bus_reg):
> /axi@600000000/mdio@6110102f8: simple-bus unit address format error,
> expected "6110102d4"
>
> Fixes: d0f482bb06f9 ("arm64: dts: sparx5: Add the Sparx5 switch
> node")
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>
> ---
>
> Not tested on hardware
> ---
> arch/arm64/boot/dts/microchip/sparx5.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/microchip/sparx5.dtsi
> b/arch/arm64/boot/dts/microchip/sparx5.dtsi
> index 24075cd91420..5d820da8c69d 100644
> --- a/arch/arm64/boot/dts/microchip/sparx5.dtsi
> +++ b/arch/arm64/boot/dts/microchip/sparx5.dtsi
> @@ -447,7 +447,7 @@ mdio2: mdio@6110102f8 {
> pinctrl-names = "default";
> #address-cells = <1>;
> #size-cells = <0>;
> - reg = <0x6 0x110102d4 0x24>;
> + reg = <0x6 0x110102f8 0x24>;
> };
>
> mdio3: mdio@61101031c {
> --
> 2.34.1
>
I did a check of our current Sparx5 EVBs and none of them uses
controller 2 in any revision, so this is probably why it has not come
up before, so as it stands we have no platform to test this change on
currently.
Besides that the change looks good to me.
Best Regards
Steen
Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
^ permalink raw reply
* RE: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support
From: Peng Fan @ 2024-04-02 14:01 UTC (permalink / raw)
To: Andy Shevchenko, Cristian Marussi
Cc: Peng Fan (OSS), Sudeep Holla, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Linus Walleij, Dan Carpenter,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-gpio@vger.kernel.org, Oleksii Moisieiev
In-Reply-To: <CAHp75VdAaTeQ_Ag3gd0s9UfT=kAT2hwibeJ9-YFXJx4z=R3e+g@mail.gmail.com>
> Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> protocol basic support
>
> On Tue, Apr 2, 2024 at 10:48 AM Cristian Marussi
> <cristian.marussi@arm.com> wrote:
> > On Sun, Mar 31, 2024 at 01:44:28PM +0000, Peng Fan wrote:
> > > > Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti:
>
> ...
>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/scmi_protocol.h> #include <linux/slab.h>
> > > >
> > > > This is semi-random list of headers. Please, follow IWYU principle
> > > > (include what you use). There are a lot of inclusions I see
> > > > missing (just in the context of this page I see bits.h, types.h, and
> asm/byteorder.h).
> > >
> > > Is there any documentation about this requirement?
> > > Some headers are already included by others.
>
> The documentation here is called "a common sense".
> The C language is built like this and we expect that nobody will invest into the
> dependency hell that we have already, that's why IWYU principle, please
> follow it.
>
> > Andy made (mostly) the same remarks on this same patch ~1-year ago on
> > this same patch while it was posted by Oleksii.
> >
> > And I told that time that most of the remarks around devm_ usage were
> > wrong due to how the SCMI core handles protocol initialization (using
> > a devres group transparently).
> >
> > This is what I answered that time.
> >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > .kernel.org%2Flinux-arm-kernel%2FZJ78hBcjAhiU%2BZBO%40e120937-
> lin%2F%2
> >
> 3t&data=05%7C02%7Cpeng.fan%40nxp.com%7C3f8c12062db048608e2a08d
> c5315bed
> >
> 0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6384766000583
> 40430%7CUn
> >
> known%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6I
> k1haW
> >
> wiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=Whn3ehZjXy%2BcKG4irlWjQ6
> K3HF%2FofD
> > Yu7j0Lrm8dN5k%3D&reserved=0
> >
> > I wont repeat myself, but, in a nutshell the memory allocation like it
> > is now is fine: a bit happens via devm_ at protocol initialization,
> > the other is doe via explicit kmalloc at runtime and freed via kfree
> > at remove time (if needed...i.e. checking the present flag of some
> > structs)
>
> This sounds like a mess. devm_ is expected to be used only for the
> ->probe() stage, otherwise you may consider cleanup.h (__free() macro)
> to have automatic free at the paths where memory is not needed.
>
> And the function naming doesn't suggest that you have a probe-remove pair.
> Moreover, if the init-deinit part is called in the probe-remove, the devm_
> must not be mixed with non-devm ones, as it breaks the order and leads to
> subtle mistakes.
I am new to __free() honestly. I'll let Cristian and Sudeep to comment on
what should I do for v8.
Thanks,
Peng.
>
> > I'll made further remarks on v7 that you just posted.
>
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply
* RE: [PATCH v7 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support
From: Peng Fan @ 2024-04-02 14:04 UTC (permalink / raw)
To: Cristian Marussi, Peng Fan (OSS)
Cc: Sudeep Holla, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Linus Walleij, Dan Carpenter,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-gpio@vger.kernel.org, Oleksii Moisieiev
In-Reply-To: <Zgvd7npz1jdJSu-b@pluto>
> Subject: Re: [PATCH v7 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> protocol basic support
>
> On Tue, Apr 02, 2024 at 10:22:23AM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Add basic implementation of the SCMI v3.2 pincontrol protocol.
> >
>
> Hi,
>
>
> > 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>
> > ---
>
> [snip]
>
>
> > +struct scmi_settings_get_ipriv {
> > + u32 selector;
> > + enum scmi_pinctrl_selector_type type;
> > + bool get_all;
> > + enum scmi_pinctrl_conf_type *config_types;
> > + u32 *config_values;
> > +};
> > +
> > +static void
> > +iter_pinctrl_settings_get_prepare_message(void *message, u32 desc_index,
> > + const void *priv)
> > +{
> > + struct scmi_msg_settings_get *msg = message;
> > + const struct scmi_settings_get_ipriv *p = priv;
> > + u32 attributes;
> > +
> > + attributes = FIELD_PREP(SELECTOR_MASK, p->type);
> > +
> > + if (p->get_all) {
> > + attributes |= FIELD_PREP(CONFIG_FLAG_MASK, 1) |
> > + FIELD_PREP(SKIP_CONFIGS_MASK, desc_index);
> > + } else {
> > + attributes |= FIELD_PREP(CONFIG_TYPE_MASK, p-
> >config_types[0]);
> > + }
> > +
> > + msg->attributes = cpu_to_le32(attributes);
> > + msg->identifier = cpu_to_le32(p->selector); }
> > +
> > +static int
> > +iter_pinctrl_settings_get_update_state(struct scmi_iterator_state *st,
> > + const void *response, void *priv) {
> > + const struct scmi_resp_settings_get *r = response;
> > + struct scmi_settings_get_ipriv *p = priv;
> > +
> > + if (p->get_all) {
> > + st->num_returned = le32_get_bits(r->num_configs,
> GENMASK(7, 0));
> > + st->num_remaining = le32_get_bits(r->num_configs,
> GENMASK(31, 24));
> > + } else {
> > + st->num_returned = 1;
> > + st->num_remaining = 0;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +iter_pinctrl_settings_get_process_response(const struct
> scmi_protocol_handle *ph,
> > + const void *response,
> > + struct scmi_iterator_state *st,
> > + void *priv)
> > +{
> > + const struct scmi_resp_settings_get *r = response;
> > + struct scmi_settings_get_ipriv *p = priv;
> > + u32 type = le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7,
> 0));
> > + u32 val = le32_to_cpu(r->configs[st->loop_idx * 2 + 1]);
> > +
> > + if (p->get_all) {
> > + p->config_types[st->desc_index + st->loop_idx] = type;
> > + } else {
> > + if (p->config_types[0] != type)
> > + return -EINVAL;
> > + }
> > +
> > + p->config_values[st->desc_index + st->loop_idx] = val;
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +scmi_pinctrl_settings_get(const struct scmi_protocol_handle *ph, u32
> selector,
> > + enum scmi_pinctrl_selector_type type,
> > + enum scmi_pinctrl_conf_type config_type,
> > + u32 *config_value, bool get_all) {
> > + int ret;
> > + void *iter;
> > + struct scmi_iterator_ops ops = {
> > + .prepare_message =
> iter_pinctrl_settings_get_prepare_message,
> > + .update_state = iter_pinctrl_settings_get_update_state,
> > + .process_response =
> iter_pinctrl_settings_get_process_response,
> > + };
> > + struct scmi_settings_get_ipriv ipriv = {
> > + .selector = selector,
> > + .type = type,
> > + .get_all = get_all,
> > + .config_types = &config_type,
> > + .config_values = config_value,
> > + };
> > +
> > + if (!config_value || type == FUNCTION_TYPE)
> > + return -EINVAL;
> > +
> > + ret = scmi_pinctrl_validate_id(ph, selector, type);
> > + if (ret)
> > + return ret;
> > +
> > + iter = ph->hops->iter_response_init(ph, &ops, SCMI_PIN_OEM_END,
> > + PINCTRL_SETTINGS_GET,
> > + sizeof(struct
> scmi_msg_settings_get),
> > + &ipriv);
> > + if (IS_ERR(iter))
> > + return PTR_ERR(iter);
> > +
> > + return ph->hops->iter_response_run(iter);
> > +}
> > +
> > +static int scmi_pinctrl_settings_get_one(const struct scmi_protocol_handle
> *ph,
> > + u32 selector,
> > + enum scmi_pinctrl_selector_type
> type,
> > + enum scmi_pinctrl_conf_type
> config_type,
> > + u32 *config_value)
> > +{
> > + return scmi_pinctrl_settings_get(ph, selector, type, config_type,
> > + config_value, false);
> > +}
> > +
> > +static int scmi_pinctrl_settings_get_all(const struct scmi_protocol_handle
> *ph,
> > + u32 selector,
> > + enum scmi_pinctrl_selector_type
> type,
> > + enum scmi_pinctrl_conf_type
> config_type,
> > + u32 *config_value)
> > +{
> > + return scmi_pinctrl_settings_get(ph, selector, type, config_type,
> > + config_value, true);
> > +}
> > +
>
> If you generalize the scmi_pinctrl_settings_get() and reintroduce a
> .settings_get_all() ops (even though unused by pinctrl driver, I am fine with
> this..), you should take care to pass as an input parameter NOT only the array
> of config_values BUT also an array of config_types since you could get back
> up to 256 OEM types: for this reason you will need also to pass to
> scmi_pinctrl_settings_get() an input param that specifies the sizes of the
> 2 array input params (in order to avoid oveflows) AND use that same inout
> param also as an output param to report at the end how many OEM types
> were effectively found and returned....
>
> IOW, I did this on top of your V7 to make the settings_get_all work:
>
> ---8<---
> diff --git a/drivers/firmware/arm_scmi/pinctrl.c
> b/drivers/firmware/arm_scmi/pinctrl.c
> index b75af1dd75fa..f4937af66c4d 100644
> --- a/drivers/firmware/arm_scmi/pinctrl.c
> +++ b/drivers/firmware/arm_scmi/pinctrl.c
> @@ -317,6 +317,7 @@ struct scmi_settings_get_ipriv {
> u32 selector;
> enum scmi_pinctrl_selector_type type;
> bool get_all;
> + unsigned int *nr_configs;
> enum scmi_pinctrl_conf_type *config_types;
> u32 *config_values;
> };
> @@ -379,6 +380,7 @@ iter_pinctrl_settings_get_process_response(const
> struct scmi_protocol_handle *ph
> }
>
> p->config_values[st->desc_index + st->loop_idx] = val;
> + ++*p->nr_configs;
>
> return 0;
> }
> @@ -386,11 +388,13 @@ iter_pinctrl_settings_get_process_response(const
> struct scmi_protocol_handle *ph static int scmi_pinctrl_settings_get(const
> struct scmi_protocol_handle *ph, u32 selector,
> enum scmi_pinctrl_selector_type type,
> - enum scmi_pinctrl_conf_type config_type,
> - u32 *config_value, bool get_all)
> + unsigned int *nr_configs,
> + enum scmi_pinctrl_conf_type *config_types,
> + u32 *config_values)
> {
> int ret;
> void *iter;
> + unsigned int max_configs = *nr_configs;
> struct scmi_iterator_ops ops = {
> .prepare_message =
> iter_pinctrl_settings_get_prepare_message,
> .update_state = iter_pinctrl_settings_get_update_state,
> @@ -399,19 +403,22 @@ scmi_pinctrl_settings_get(const struct
> scmi_protocol_handle *ph, u32 selector,
> struct scmi_settings_get_ipriv ipriv = {
> .selector = selector,
> .type = type,
> - .get_all = get_all,
> - .config_types = &config_type,
> - .config_values = config_value,
> + .get_all = (max_configs > 1),
> + .nr_configs = nr_configs,
> + .config_types = config_types,
> + .config_values = config_values,
> };
>
> - if (!config_value || type == FUNCTION_TYPE)
> + if (!config_types || !config_values || type == FUNCTION_TYPE)
> return -EINVAL;
>
> ret = scmi_pinctrl_validate_id(ph, selector, type);
> if (ret)
> return ret;
>
> - iter = ph->hops->iter_response_init(ph, &ops, SCMI_PIN_OEM_END,
> + /* Prepare to count returned configs */
> + *nr_configs = 0;
> + iter = ph->hops->iter_response_init(ph, &ops, max_configs,
> PINCTRL_SETTINGS_GET,
> sizeof(struct
> scmi_msg_settings_get),
> &ipriv);
> @@ -427,18 +434,24 @@ static int scmi_pinctrl_settings_get_one(const
> struct scmi_protocol_handle *ph,
> enum scmi_pinctrl_conf_type
> config_type,
> u32 *config_value)
> {
> - return scmi_pinctrl_settings_get(ph, selector, type, config_type,
> - config_value, false);
> + unsigned int nr_configs = 1;
> +
> + return scmi_pinctrl_settings_get(ph, selector, type, &nr_configs,
> + &config_type, config_value);
> }
>
> static int scmi_pinctrl_settings_get_all(const struct scmi_protocol_handle
> *ph,
> u32 selector,
> enum scmi_pinctrl_selector_type
> type,
> - enum scmi_pinctrl_conf_type
> config_type,
> - u32 *config_value)
> + unsigned int *nr_configs,
> + enum scmi_pinctrl_conf_type
> *config_types,
> + u32 *config_values)
> {
> - return scmi_pinctrl_settings_get(ph, selector, type, config_type,
> - config_value, true);
> + if (!nr_configs || *nr_configs == 0)
> + return -EINVAL;
> +
> + return scmi_pinctrl_settings_get(ph, selector, type, nr_configs,
> + config_types, config_values);
> }
>
> static int
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index abaf6122ea37..7915792efd81 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -882,8 +882,9 @@ struct scmi_pinctrl_proto_ops {
> int (*settings_get_all)(const struct scmi_protocol_handle *ph,
> u32 selector,
> enum scmi_pinctrl_selector_type type,
> - enum scmi_pinctrl_conf_type config_type,
> - u32 *config_value);
> + unsigned int *nr_configs,
> + enum scmi_pinctrl_conf_type *config_types,
> + u32 *config_values);
> int (*settings_conf)(const struct scmi_protocol_handle *ph,
> u32 selector, enum scmi_pinctrl_selector_type
> type,
> unsigned int nr_configs,
> --->8-----
>
> Please check if this addition sounds good to you and integrate into v8
> eventually...
Thanks for helping on this, I will included your changes, and your
Co-developed-by tag if you not mind.
Thanks,
Peng.
>
> Thanks,
> Cristian
^ permalink raw reply
* Re: [PATCH v6 0/4] fpga: xilinx-selectmap: add new driver
From: Charles Perry @ 2024-04-02 13:57 UTC (permalink / raw)
To: Xu Yilun
Cc: mdf, Allen VANDIVER, Brian CODY, hao wu, yilun xu, Tom Rix,
Rob Herring, krzysztof kozlowski+dt, Conor Dooley, Michal Simek,
linux-fpga, devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <Zgl4I6rykg7shI2K@yilunxu-OptiPlex-7050>
On Mar 31, 2024, at 10:50 AM, Xu Yilun yilun.xu@linux.intel.com wrote:
> On Thu, Mar 21, 2024 at 06:04:32PM -0400, Charles Perry wrote:
>> Hello,
>>
>> This patchset adds a new driver for the 7 series FPGA's SelectMAP
>> interface.
>>
>> The SelectMAP interface shares a common GPIO protocol with the SPI
>> interface which is already in the kernel (drivers/fpga/xilinx-spi.c).
>> The approach proposed in this patchset is to refactor xilinx-spi.c into
>> xilinx-core.c which would handle the common GPIO protocol. This is then
>> used to build two drivers, the already existing xilinx-spi.c driver and
>> a newly added xilinx-selectmap.c driver.
>>
>> The SelectMAP driver proposed only supports 8 bit mode. This is because
>> the 16 and 32 bits mode have limitations with regards to compressed
>> bitstream support as well as introducing endianness considerations.
>>
>> I'm testing xilinx-selectmap.c on a custom i.MX6 board connected to an
>> Artix 7 FPGA. Flashing a 913K bitstream takes 0.44 seconds.
>>
>> Changes since v5: (from Yilun review)
>> * xilinx-core.h: remove private fields kernel-doc
>> * xilinx-spi.c: rename conf into core in xilinx_spi_probe
>> * xilinx-core.c: introduce the new gpio names in patch 4/4
>> * xilinx-core.c: remove kernel-doc on xilinx_core_devm_gpiod_get()
>> * xilinx-selectmap.c:
>> * reorder includes in alphabetical order
>> * xilinx_selectmap_probe(): remove unused resource *r variable
>> * xilinx_selectmap_probe(): use a single gpio_desc* temporary
>> * xilinx_selectmap_probe(): declare variables in reverse xmas tree
>>
>> Changes since v4: (from Yilun and Krzysztof review)
>> * xilinx-core: use sizeof() instead of hardcoded immediate
>> * xilinx-core: fix module compilation (EXPORT_SYMBOL_GPL, MODULE_LICENSE,
>> MODULE_AUTHOR, MODULE_DESCRIPTION)
>> * xilinx-core: add private/public qualifiers for struct xilinx_fpga_core
>> * xilinx-spi: remove struct xilinx_spi_conf. This struct isn't needed as
>> the struct spi_device* can be retrieved from the struct device*.
>> * dt-bindings: remove usage of "_b" and "-b" for the new driver. We
>> agreed that the spi and selectmap driver will use different bindings
>> which will be handled by the driver core and that the legacy names will
>> be used only for the spi compatible.
>> * xilinx-core: select between prog/init and prog_b/init-b
>>
>> Changes since v3: (from Rob Herring review)
>> * Fix an error in the DT binding example compatible.
>> * Drop the renaming of "prog_b" to "prog" and "init-b" to "init".
>> Patches 2 and 3 are removed.
>>
>> Changes since v2:
>> * Inserted patch 2 and 3 which rename "prog_b" and "init-b" into "prog"
>> and "init" for the SPI driver.
>> * From Krzysztof Kozlowski review's:
>> * Use more specific compatible names
>> * Remove other missing occurences of the slave word missed in v2.
>> * From Xu Yilun review's:
>> * Fix vertical whitespace in get_done_gpio().
>> * Combine write() and write_one_dummy_byte() together.
>> * Eliminate most of the xilinx_core_probe() arguments, the driver
>> needs to populate those directly into the xilinx_fpga_core struct.
>> Added some documentation to struct xilinx_fpga_core to clarify
>> this.
>> * Removed typedefs from xilinx-core.h.
>> * Moved null checks in xilinx_core_probe() to first patch.
>> * Move csi_b and rdwr_b out of xilinx_selectmap_conf as they are not
>> used out of the probe function.
>>
>> Changes since v1: (from Krzysztof Kozlowski review's)
>> * Use more conventional names for gpio DT bindings
>> * fix example in DT bindings
>> * add mc-peripheral-props.yaml to DT bindings
>> * fix various formatting mistakes
>> * Remove all occurences of the "slave" word.
>>
>> Charles Perry (4):
>> fpga: xilinx-spi: extract a common driver core
>> dt-bindings: fpga: xlnx,fpga-selectmap: add DT schema
>> fpga: xilinx-selectmap: add new driver
>> xilinx-core: add new gpio names for prog and init
>>
>> .../bindings/fpga/xlnx,fpga-selectmap.yaml | 86 +++++++
>> drivers/fpga/Kconfig | 12 +
>> drivers/fpga/Makefile | 2 +
>> drivers/fpga/xilinx-core.c | 229 ++++++++++++++++++
>> drivers/fpga/xilinx-core.h | 27 +++
>> drivers/fpga/xilinx-selectmap.c | 95 ++++++++
>> drivers/fpga/xilinx-spi.c | 224 ++---------------
>> 7 files changed, 466 insertions(+), 209 deletions(-)
>> create mode 100644
>> Documentation/devicetree/bindings/fpga/xlnx,fpga-selectmap.yaml
>> create mode 100644 drivers/fpga/xilinx-core.c
>> create mode 100644 drivers/fpga/xilinx-core.h
>> create mode 100644 drivers/fpga/xilinx-selectmap.c
>
> Applied this series to for-next with a nit.
>
> Thanks,
> Yilun
>
Thanks again for all the good reviews Yilun.
Regards,
Charles
>>
>> --
>> 2.43.0
^ permalink raw reply
* Re: [PATCH v7 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support
From: Andy Shevchenko @ 2024-04-02 14:05 UTC (permalink / raw)
To: Peng Fan
Cc: Peng Fan (OSS), Sudeep Holla, Cristian Marussi, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Linus Walleij, Dan Carpenter,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-gpio@vger.kernel.org, Oleksii Moisieiev
In-Reply-To: <DU0PR04MB941780B4C28DB353C64966F8883E2@DU0PR04MB9417.eurprd04.prod.outlook.com>
On Tue, Apr 02, 2024 at 01:27:19PM +0000, Peng Fan wrote:
> > On Tue, Apr 02, 2024 at 10:22:23AM +0800, Peng Fan (OSS) wrote:
...
> > > +#include <linux/module.h>
> > > +#include <linux/scmi_protocol.h>
> > > +#include <linux/slab.h>
> >
> > Please, follow IWYU principle, a lot of headers are missed.
>
> ok. I will try to figure out. BTW, is there an easy way to filter
> out what is missed?
For you is much easier than to me as I'm not familiar with the code.
Basically you should know what you wrote :-)
But if you are asking about tooling, we would appreciate when somebody comes
with a such.
> > > +#include "common.h"
> > > +#include "protocols.h"
...
> > > + ret = scmi_pinctrl_get_pin_info(ph, selector,
> > > + &pi->pins[selector]);
> >
> > It's netter as a single line.
>
> I try to follow 80 max chars per SCMI coding style. If Sudeep and Cristian
> is ok, I could use a single line.
It's minor, but even before relaxation of 80 limit it was and is mentioned
in the documentation that you may go over if it increases readability.
> > > + if (ret)
> > > + return ret;
> > > + }
...
> > > +static int scmi_pinctrl_protocol_init(const struct
> > > +scmi_protocol_handle *ph) {
> > > + int ret;
> > > + u32 version;
> > > + struct scmi_pinctrl_info *pinfo;
> > > +
> > > + ret = ph->xops->version_get(ph, &version);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + dev_dbg(ph->dev, "Pinctrl Version %d.%d\n",
> > > + PROTOCOL_REV_MAJOR(version),
> > PROTOCOL_REV_MINOR(version));
> > > +
> > > + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
> > > + if (!pinfo)
> > > + return -ENOMEM;
> > > +
> > > + ret = scmi_pinctrl_attributes_get(ph, pinfo);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + pinfo->pins = devm_kcalloc(ph->dev, pinfo->nr_pins,
> > > + sizeof(*pinfo->pins), GFP_KERNEL);
> > > + if (!pinfo->pins)
> > > + return -ENOMEM;
> > > +
> > > + pinfo->groups = devm_kcalloc(ph->dev, pinfo->nr_groups,
> > > + sizeof(*pinfo->groups), GFP_KERNEL);
> > > + if (!pinfo->groups)
> > > + return -ENOMEM;
> > > +
> > > + pinfo->functions = devm_kcalloc(ph->dev, pinfo->nr_functions,
> > > + sizeof(*pinfo->functions),
> > GFP_KERNEL);
> > > + if (!pinfo->functions)
> > > + return -ENOMEM;
> > > +
> > > + pinfo->version = version;
> > > +
> > > + return ph->set_priv(ph, pinfo, version);
> >
> > Same comments as per previous version. devm_ here is simply wrong.
> > It breaks the order of freeing resources.
> >
> > I.o.w. I see *no guarantee* that these init-deinit functions will be properly
> > called from the respective probe-remove. Moreover the latter one may also
> > have its own devm allocations (which are rightfully placed) and you get
> > completely out of control the resource management.
>
> I see an old thread.
> https://lore.kernel.org/linux-arm-kernel/ZJ78hBcjAhiU+ZBO@e120937-lin/#t
>
> The free in deinit is not to free the ones alloced in init, it is to free the ones
> alloced such as in scmi_pinctrl_get_function_info
Even messier than I thought. For bare minimum these two should be documented
and renamed accordingly that no-one will think that deinit is a counter part
of init.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v7 4/4] pinctrl: Implementation of the generic scmi-pinctrl driver
From: Andy Shevchenko @ 2024-04-02 14:09 UTC (permalink / raw)
To: Peng Fan
Cc: Peng Fan (OSS), Sudeep Holla, Cristian Marussi, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Linus Walleij, Dan Carpenter,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-gpio@vger.kernel.org, Oleksii Moisieiev
In-Reply-To: <DU0PR04MB9417D0D33573E99D440D7BD7883E2@DU0PR04MB9417.eurprd04.prod.outlook.com>
On Tue, Apr 02, 2024 at 01:59:19PM +0000, Peng Fan wrote:
> > On Tue, Apr 02, 2024 at 10:22:24AM +0800, Peng Fan (OSS) wrote:
...
> > > +#include <linux/device.h>
> > > +#include <linux/err.h>
> > > +#include <linux/module.h>
> > > +#include <linux/seq_file.h>
> > > +#include <linux/scmi_protocol.h>
> > > +#include <linux/slab.h>
> >
> > Missing headers.
>
> Not sure there is an easy way to filter out what is missed.
And?..
You are the author, not me. You know your code much better and
it will be quite easy to perform. I may miss things, but reading
briefly the 1000 lines and get what headers are required takes
no more than half an hour.
(Tools that help me, in case I don't remember by heart, are
`cscope` and `git grep ...`.)
...
> > > + ret = pinctrl_ops->name_get(pmx->ph, i, PIN_TYPE,
> > &pins[i].name);
> > > + if (ret)
> >
> > How does the cleanup work for the previously assigned pin names? Is it
> > needed?
>
> No need. The "name" memory region is allocated in firmware pinctrl
> Protocol init phase.
>
> > Maybe a comment?
>
> ok. As below.
> /*
> * The region for name is handled by the scmi firmware driver,
> * no need free here
> */
LGTM.
> > > + return dev_err_probe(pmx->dev, ret,
> > > + "Can't get name for pin %d", i);
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v7 4/4] pinctrl: Implementation of the generic scmi-pinctrl driver
From: Dan Carpenter @ 2024-04-02 14:09 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Peng Fan (OSS), Sudeep Holla, Cristian Marussi, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Linus Walleij,
linux-arm-kernel, linux-kernel, devicetree, linux-gpio, Peng Fan,
Oleksii Moisieiev
In-Reply-To: <ZgwGpZ6S13vjk8jh@smile.fi.intel.com>
On Tue, Apr 02, 2024 at 04:22:45PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 02, 2024 at 10:22:24AM +0800, Peng Fan (OSS) wrote:
> > +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;
> > +
> > + 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.
> > + */
>
> This is fragile, but at least it is documented.
>
It was never clear to me where the crash would happen if npins was zero.
Does some part of pinctrl internals assume we have at least one pin?
It's nice to be able to allocate zero element arrays and generally it
works well in the kernel. The one common bug with zero element arrays
has to do with strings. Something like this (garbage) example:
str = kmalloc(n_char, GFP_KERNEL);
copy_from_user(str, user_ptr, n_char);
str[n_char - 1] = '\0';
If the str is zero bytes long it will lead to an Oops when we add a NUL
terminator.
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH 0/3] media: i2c: Add imx283 camera sensor driver
From: Konstantin Ryabitsev @ 2024-04-02 14:15 UTC (permalink / raw)
To: Umang Jain
Cc: Kieran Bingham, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
Sakari Ailus, linux-media, devicetree, linux-arm-kernel,
linux-kernel, Rob Herring, Laurent Pinchart
In-Reply-To: <d46380d7-e296-445b-a294-b0231ea45518@ideasonboard.com>
On Tue, Apr 02, 2024 at 02:01:19PM +0530, Umang Jain wrote:
> Hi all,
>
> PLease ignore the series, I was testing/learning the b4 tool.
>
> I did pass --offline-mode but it has sent the patches anyway :-//
Hm... I see why this would be confusing and I will try to fix this.
(the --offline-mode flag is to prevent b4 from querying lore.kernel.org, but
it should also bubble up to other functionality that calls remote services,
like b4 send).
-K
^ permalink raw reply
* Re: [PATCH v2 2/3] dt-bindings: phy: Add i.MX8Q HSIO SerDes PHY binding
From: Frank Li @ 2024-04-02 14:23 UTC (permalink / raw)
To: Richard Zhu
Cc: vkoul, kishon, robh+dt, krzysztof.kozlowski+dt, conor+dt,
linux-phy, devicetree, linux-arm-kernel, linux-kernel, kernel,
imx
In-Reply-To: <1712036704-21064-3-git-send-email-hongxing.zhu@nxp.com>
On Tue, Apr 02, 2024 at 01:45:03PM +0800, Richard Zhu wrote:
> Add i.MX8QM and i.MX8QXP HSIO SerDes PHY binding.
> - Use the controller ID to specify which controller is binded to the
> PHY.
> - Introduce one HSIO configuration, mandatory required to set
> "PCIE_AB_SELECT" and "PHY_X1_EPCS_SEL" during the initialization.
>
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
You missed all conor's comments.
Please double check v1's comments.
Frank
> ---
> .../bindings/phy/fsl,imx8q-hsio.yaml | 143 ++++++++++++++++++
> 1 file changed, 143 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/phy/fsl,imx8q-hsio.yaml
>
> diff --git a/Documentation/devicetree/bindings/phy/fsl,imx8q-hsio.yaml b/Documentation/devicetree/bindings/phy/fsl,imx8q-hsio.yaml
> new file mode 100644
> index 000000000000..506551d4d94a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/fsl,imx8q-hsio.yaml
> @@ -0,0 +1,143 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/fsl,imx8q-hsio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Freescale i.MX8Q SoC series HSIO SERDES PHY
> +
> +maintainers:
> + - Richard Zhu <hongxing.zhu@nxp.com>
> +
> +properties:
> + compatible:
> + enum:
> + - fsl,imx8qxp-serdes
> + - fsl,imx8qm-serdes
> + reg:
> + minItems: 4
> + maxItems: 4
> +
> + "#phy-cells":
> + const: 3
> + description: |
> + The first number defines the ID of the PHY contained in the HSIO macro.
> + The second defines controller ID binded to the PHY. The third defines the
> + HSIO configuratons refer to the different use cases. They are defined in
> + dt-bindings/phy/phy-imx8-pcie.h
> +
> + reg-names:
> + items:
> + - const: reg
> + - const: phy
> + - const: ctrl
> + - const: misc
> +
> + clocks:
> + minItems: 5
> + maxItems: 14
> +
> + clock-names:
> + minItems: 5
> + maxItems: 14
> +
> + fsl,refclk-pad-mode:
> + description: |
> + Specifies the mode of the refclk pad used. It can be UNUSED(PHY
> + refclock is derived from SoC internal source), INPUT(PHY refclock
> + is provided externally via the refclk pad) or OUTPUT(PHY refclock
> + is derived from SoC internal source and provided on the refclk pad).
> + Refer include/dt-bindings/phy/phy-imx8-pcie.h for the constants
> + to be used.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [ 0, 1, 2 ]
I remember needn't enum because there are header file.
> +
> + power-domains:
> + description: |
> + i.MX8Q HSIO SerDes power domains. i.MX8QXP has one SerDes power domains.
> + And i.MX8QM has two.
> + minItems: 1
> + maxItems: 2
> +
> +required:
> + - compatible
> + - reg
> + - "#phy-cells"
> + - clocks
> + - clock-names
> + - fsl,refclk-pad-mode
> +
> +allOf:
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - fsl,imx8qxp-serdes
> + then:
> + properties:
> + clock-names:
> + items:
> + - const: apb_pclk0
> + - const: pclk0
> + - const: phy0_crr
> + - const: ctl0_crr
> + - const: misc_crr
> + power-domains:
> + minItems: 1
> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - fsl,imx8qm-serdes
> + then:
> + properties:
> + clock-names:
> + items:
> + - const: pclk0
> + - const: pclk1
> + - const: apb_pclk0
> + - const: apb_pclk1
> + - const: pclk2
> + - const: epcs_tx
> + - const: epcs_rx
> + - const: apb_pclk2
> + - const: phy0_crr
> + - const: phy1_crr
> + - const: ctl0_crr
> + - const: ctl1_crr
> + - const: ctl2_crr
> + - const: misc_crr
> + power-domains:
> + minItems: 2
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/imx8-clock.h>
> + #include <dt-bindings/clock/imx8-lpcg.h>
> + #include <dt-bindings/firmware/imx/rsrc.h>
> + #include <dt-bindings/phy/phy-imx8-pcie.h>
> +
> + serdes: phy@5f1a0000 {
No "serdes".
> + compatible = "fsl,imx8qxp-serdes";
> + reg = <0x5f1a0000 0x10000>,
> + <0x5f120000 0x10000>,
> + <0x5f140000 0x10000>,
> + <0x5f160000 0x10000>;
> + reg-names = "reg", "phy", "ctrl", "misc";
> + clocks = <&phyx1_lpcg IMX_LPCG_CLK_0>,
> + <&phyx1_lpcg IMX_LPCG_CLK_4>,
> + <&phyx1_crr1_lpcg IMX_LPCG_CLK_4>,
> + <&pcieb_crr3_lpcg IMX_LPCG_CLK_4>,
> + <&misc_crr5_lpcg IMX_LPCG_CLK_4>;
> + clock-names = "apb_pclk0", "pclk0", "phy0_crr", "ctl0_crr",
> + "misc_crr";
> + power-domains = <&pd IMX_SC_R_SERDES_1>;
> + #phy-cells = <3>;
> + status = "disabled";
needn't status = "disabled".
> + };
> +...
> --
> 2.37.1
>
^ permalink raw reply
* Re: [PATCH v1 6/6] clk: meson: a1: add Amlogic A1 CPU clock controller driver
From: Jerome Brunet @ 2024-04-02 14:11 UTC (permalink / raw)
To: Dmitry Rokosov
Cc: Jerome Brunet, neil.armstrong, mturquette, sboyd, robh+dt,
krzysztof.kozlowski+dt, khilman, martin.blumenstingl, kernel,
rockosov, linux-amlogic, linux-clk, devicetree, linux-kernel,
linux-arm-kernel
In-Reply-To: <20240402110538.ayectwxnhlu6o65d@CAB-WSD-L081021>
On Tue 02 Apr 2024 at 14:05, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
> Hello Jerome,
>
> On Tue, Apr 02, 2024 at 11:35:49AM +0200, Jerome Brunet wrote:
>>
>> On Fri 29 Mar 2024 at 23:58, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
>>
>> > The CPU clock controller plays a general role in the Amlogic A1 SoC
>> > family by generating CPU clocks. As an APB slave module, it offers the
>> > capability to inherit the CPU clock from two sources: the internal fixed
>> > clock known as 'cpu fixed clock' and the external input provided by the
>> > A1 PLL clock controller, referred to as 'syspll'.
>> >
>> > It is important for the driver to handle cpu_clk rate switching
>> > effectively by transitioning to the CPU fixed clock to avoid any
>> > potential execution freezes.
>> >
>> > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
>> > ---
>> > drivers/clk/meson/Kconfig | 10 ++
>> > drivers/clk/meson/Makefile | 1 +
>> > drivers/clk/meson/a1-cpu.c | 324 +++++++++++++++++++++++++++++++++++++
>> > drivers/clk/meson/a1-cpu.h | 16 ++
>> > 4 files changed, 351 insertions(+)
>> > create mode 100644 drivers/clk/meson/a1-cpu.c
>> > create mode 100644 drivers/clk/meson/a1-cpu.h
>> >
>> > diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>> > index 80c4a18c83d2..148d4495eee3 100644
>> > --- a/drivers/clk/meson/Kconfig
>> > +++ b/drivers/clk/meson/Kconfig
>> > @@ -111,6 +111,16 @@ config COMMON_CLK_AXG_AUDIO
>> > Support for the audio clock controller on AmLogic A113D devices,
>> > aka axg, Say Y if you want audio subsystem to work.
>> >
>> > +config COMMON_CLK_A1_CPU
>> > + tristate "Amlogic A1 SoC CPU controller support"
>> > + depends on ARM64
>> > + select COMMON_CLK_MESON_REGMAP
>> > + select COMMON_CLK_MESON_CLKC_UTILS
>> > + help
>> > + Support for the CPU clock controller on Amlogic A113L based
>> > + device, A1 SoC Family. Say Y if you want A1 CPU clock controller
>> > + to work.
>> > +
>> > config COMMON_CLK_A1_PLL
>> > tristate "Amlogic A1 SoC PLL controller support"
>> > depends on ARM64
>> > diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>> > index 4968fc7ad555..2a06eb0303d6 100644
>> > --- a/drivers/clk/meson/Makefile
>> > +++ b/drivers/clk/meson/Makefile
>> > @@ -18,6 +18,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_AUDIO_RSTC) += meson-audio-rstc.o
>> >
>> > obj-$(CONFIG_COMMON_CLK_AXG) += axg.o axg-aoclk.o
>> > obj-$(CONFIG_COMMON_CLK_AXG_AUDIO) += axg-audio.o
>> > +obj-$(CONFIG_COMMON_CLK_A1_CPU) += a1-cpu.o
>> > obj-$(CONFIG_COMMON_CLK_A1_PLL) += a1-pll.o
>> > obj-$(CONFIG_COMMON_CLK_A1_PERIPHERALS) += a1-peripherals.o
>> > obj-$(CONFIG_COMMON_CLK_A1_AUDIO) += a1-audio.o
>> > diff --git a/drivers/clk/meson/a1-cpu.c b/drivers/clk/meson/a1-cpu.c
>> > new file mode 100644
>> > index 000000000000..5f5d8ae112e5
>> > --- /dev/null
>> > +++ b/drivers/clk/meson/a1-cpu.c
>> > @@ -0,0 +1,324 @@
>> > +// SPDX-License-Identifier: GPL-2.0+
>> > +/*
>> > + * Amlogic A1 SoC family CPU Clock Controller driver.
>> > + *
>> > + * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
>> > + * Author: Dmitry Rokosov <ddrokosov@salutedevices.com>
>> > + */
>> > +
>> > +#include <linux/clk.h>
>> > +#include <linux/clk-provider.h>
>> > +#include <linux/mod_devicetable.h>
>> > +#include <linux/platform_device.h>
>> > +#include "a1-cpu.h"
>> > +#include "clk-regmap.h"
>> > +#include "meson-clkc-utils.h"
>> > +
>> > +#include <dt-bindings/clock/amlogic,a1-cpu-clkc.h>
>> > +
>> > +static u32 cpu_fsource_sel_table[] = { 0, 1, 2 };
>> > +static const struct clk_parent_data cpu_fsource_sel_parents[] = {
>> > + { .fw_name = "xtal" },
>> > + { .fw_name = "fclk_div2" },
>> > + { .fw_name = "fclk_div3" },
>> > +};
>> > +
>> > +static struct clk_regmap cpu_fsource_sel0 = {
>> > + .data = &(struct clk_regmap_mux_data) {
>> > + .offset = CPUCTRL_CLK_CTRL0,
>> > + .mask = 0x3,
>> > + .shift = 0,
>> > + .table = cpu_fsource_sel_table,
>> > + },
>> > + .hw.init = &(struct clk_init_data) {
>> > + .name = "cpu_fsource_sel0",
>> > + .ops = &clk_regmap_mux_ops,
>> > + .parent_data = cpu_fsource_sel_parents,
>> > + .num_parents = ARRAY_SIZE(cpu_fsource_sel_parents),
>> > + .flags = CLK_SET_RATE_PARENT,
>> > + },
>> > +};
>> > +
>> > +static struct clk_regmap cpu_fsource_div0 = {
>> > + .data = &(struct clk_regmap_div_data) {
>> > + .offset = CPUCTRL_CLK_CTRL0,
>> > + .shift = 4,
>> > + .width = 6,
>> > + },
>> > + .hw.init = &(struct clk_init_data) {
>> > + .name = "cpu_fsource_div0",
>> > + .ops = &clk_regmap_divider_ops,
>> > + .parent_hws = (const struct clk_hw *[]) {
>> > + &cpu_fsource_sel0.hw
>> > + },
>> > + .num_parents = 1,
>> > + .flags = CLK_SET_RATE_PARENT,
>> > + },
>> > +};
>> > +
>> > +static struct clk_regmap cpu_fsel0 = {
>> > + .data = &(struct clk_regmap_mux_data) {
>> > + .offset = CPUCTRL_CLK_CTRL0,
>> > + .mask = 0x1,
>> > + .shift = 2,
>> > + },
>> > + .hw.init = &(struct clk_init_data) {
>> > + .name = "cpu_fsel0",
>> > + .ops = &clk_regmap_mux_ops,
>> > + .parent_hws = (const struct clk_hw *[]) {
>> > + &cpu_fsource_sel0.hw,
>> > + &cpu_fsource_div0.hw,
>> > + },
>> > + .num_parents = 2,
>> > + .flags = CLK_SET_RATE_PARENT,
>> > + },
>> > +};
>> > +
>> > +static struct clk_regmap cpu_fsource_sel1 = {
>> > + .data = &(struct clk_regmap_mux_data) {
>> > + .offset = CPUCTRL_CLK_CTRL0,
>> > + .mask = 0x3,
>> > + .shift = 16,
>> > + .table = cpu_fsource_sel_table,
>> > + },
>> > + .hw.init = &(struct clk_init_data) {
>> > + .name = "cpu_fsource_sel1",
>> > + .ops = &clk_regmap_mux_ops,
>> > + .parent_data = cpu_fsource_sel_parents,
>> > + .num_parents = ARRAY_SIZE(cpu_fsource_sel_parents),
>> > + .flags = CLK_SET_RATE_PARENT,
>> > + },
>> > +};
>> > +
>> > +static struct clk_regmap cpu_fsource_div1 = {
>> > + .data = &(struct clk_regmap_div_data) {
>> > + .offset = CPUCTRL_CLK_CTRL0,
>> > + .shift = 20,
>> > + .width = 6,
>> > + },
>> > + .hw.init = &(struct clk_init_data) {
>> > + .name = "cpu_fsource_div1",
>> > + .ops = &clk_regmap_divider_ops,
>> > + .parent_hws = (const struct clk_hw *[]) {
>> > + &cpu_fsource_sel1.hw
>> > + },
>> > + .num_parents = 1,
>> > + .flags = CLK_SET_RATE_PARENT,
>> > + },
>> > +};
>> > +
>> > +static struct clk_regmap cpu_fsel1 = {
>> > + .data = &(struct clk_regmap_mux_data) {
>> > + .offset = CPUCTRL_CLK_CTRL0,
>> > + .mask = 0x1,
>> > + .shift = 18,
>> > + },
>> > + .hw.init = &(struct clk_init_data) {
>> > + .name = "cpu_fsel1",
>> > + .ops = &clk_regmap_mux_ops,
>> > + .parent_hws = (const struct clk_hw *[]) {
>> > + &cpu_fsource_sel1.hw,
>> > + &cpu_fsource_div1.hw,
>> > + },
>> > + .num_parents = 2,
>> > + .flags = CLK_SET_RATE_PARENT,
>> > + },
>> > +};
>> > +
>> > +static struct clk_regmap cpu_fclk = {
>> > + .data = &(struct clk_regmap_mux_data) {
>> > + .offset = CPUCTRL_CLK_CTRL0,
>> > + .mask = 0x1,
>> > + .shift = 10,
>> > + },
>> > + .hw.init = &(struct clk_init_data) {
>> > + .name = "cpu_fclk",
>> > + .ops = &clk_regmap_mux_ops,
>> > + .parent_hws = (const struct clk_hw *[]) {
>> > + &cpu_fsel0.hw,
>> > + &cpu_fsel1.hw,
>> > + },
>> > + .num_parents = 2,
>> > + .flags = CLK_SET_RATE_PARENT,
>> > + },
>> > +};
>> > +
>> > +static struct clk_regmap cpu_clk = {
>> > + .data = &(struct clk_regmap_mux_data) {
>> > + .offset = CPUCTRL_CLK_CTRL0,
>> > + .mask = 0x1,
>> > + .shift = 11,
>> > + },
>> > + .hw.init = &(struct clk_init_data) {
>> > + .name = "cpu_clk",
>> > + .ops = &clk_regmap_mux_ops,
>> > + .parent_data = (const struct clk_parent_data []) {
>> > + { .hw = &cpu_fclk.hw },
>> > + { .fw_name = "sys_pll", },
>> > + },
>> > + .num_parents = 2,
>> > + .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
>> > + },
>> > +};
>> > +
>> > +/* Array of all clocks registered by this provider */
>> > +static struct clk_hw *a1_cpu_hw_clks[] = {
>> > + [CLKID_CPU_FSOURCE_SEL0] = &cpu_fsource_sel0.hw,
>> > + [CLKID_CPU_FSOURCE_DIV0] = &cpu_fsource_div0.hw,
>> > + [CLKID_CPU_FSEL0] = &cpu_fsel0.hw,
>> > + [CLKID_CPU_FSOURCE_SEL1] = &cpu_fsource_sel1.hw,
>> > + [CLKID_CPU_FSOURCE_DIV1] = &cpu_fsource_div1.hw,
>> > + [CLKID_CPU_FSEL1] = &cpu_fsel1.hw,
>> > + [CLKID_CPU_FCLK] = &cpu_fclk.hw,
>> > + [CLKID_CPU_CLK] = &cpu_clk.hw,
>> > +};
>> > +
>> > +static struct clk_regmap *const a1_cpu_regmaps[] = {
>> > + &cpu_fsource_sel0,
>> > + &cpu_fsource_div0,
>> > + &cpu_fsel0,
>> > + &cpu_fsource_sel1,
>> > + &cpu_fsource_div1,
>> > + &cpu_fsel1,
>> > + &cpu_fclk,
>> > + &cpu_clk,
>> > +};
>> > +
>> > +static struct regmap_config a1_cpu_regmap_cfg = {
>> > + .reg_bits = 32,
>> > + .val_bits = 32,
>> > + .reg_stride = 4,
>> > + .max_register = CPUCTRL_CLK_CTRL1,
>> > +};
>> > +
>> > +static struct meson_clk_hw_data a1_cpu_clks = {
>> > + .hws = a1_cpu_hw_clks,
>> > + .num = ARRAY_SIZE(a1_cpu_hw_clks),
>> > +};
>> > +
>> > +struct a1_cpu_clk_nb_data {
>> > + const struct clk_ops *mux_ops;
>>
>> That's fishy ...
>>
>> > + struct clk_hw *cpu_clk;
>> > + struct notifier_block nb;
>> > + u8 parent;
>> > +};
>> > +
>> > +#define MESON_A1_CPU_CLK_GET_PARENT(nbd) \
>> > + ((nbd)->mux_ops->get_parent((nbd)->cpu_clk))
>> > +#define MESON_A1_CPU_CLK_SET_PARENT(nbd, index) \
>> > + ((nbd)->mux_ops->set_parent((nbd)->cpu_clk, index))
>>
>> ... Directly going for the mux ops ??!?? No way !
>>
>> We have a framework to handle the clocks, the whole point is to use it,
>> not bypass it !
>>
>
> I suppose you understand my approach, which is quite similar to what is
> happening in the Mediatek driver:
>
> https://elixir.bootlin.com/linux/latest/source/drivers/clk/mediatek/clk-mux.c#L295
>
> Initially, I attempted to set the parent using the clk_set_parent() API.
> However, I encountered a problem with recursive calling of the
> notifier_block. This issue arises because the parent triggers
> notifications for its children, leading to repeated calls to the
> notifier_block.
>
> I find it puzzling why I cannot call an internal function or callback
> within the internal driver context. After all, the notifier block is
> just a part of the set_rate() flow. From a global Clock Control
> Framework perspective, the context should not change.
I don't care what MTK is doing TBH. Forcefully calling ops on a clock,
hoping they are going to match with the clock type is wrong.
There is a clk_hw API. Please it.
>
>> > +
>> > +static int meson_a1_cpu_clk_notifier_cb(struct notifier_block *nb,
>> > + unsigned long event, void *data)
>> > +{
>> > + struct a1_cpu_clk_nb_data *nbd;
>> > + int ret = 0;
>> > +
>> > + nbd = container_of(nb, struct a1_cpu_clk_nb_data, nb);
>> > +
>> > + switch (event) {
>> > + case PRE_RATE_CHANGE:
>> > + nbd->parent = MESON_A1_CPU_CLK_GET_PARENT(nbd);
>> > + /* Fallback to the CPU fixed clock */
>> > + ret = MESON_A1_CPU_CLK_SET_PARENT(nbd, 0);
>> > + /* Wait for clock propagation */
>> > + udelay(100);
>> > + break;
>> > +
>> > + case POST_RATE_CHANGE:
>> > + case ABORT_RATE_CHANGE:
>> > + /* Back to the original parent clock */
>> > + ret = MESON_A1_CPU_CLK_SET_PARENT(nbd, nbd->parent);
>> > + /* Wait for clock propagation */
>> > + udelay(100);
>> > + break;
>> > +
>> > + default:
>> > + pr_warn("Unknown event %lu for %s notifier block\n",
>> > + event, clk_hw_get_name(nbd->cpu_clk));
>> > + break;
>> > + }
>> > +
>> > + return notifier_from_errno(ret);
>> > +}
>> > +
>> > +static struct a1_cpu_clk_nb_data a1_cpu_clk_nb_data = {
>> > + .mux_ops = &clk_regmap_mux_ops,
>> > + .cpu_clk = &cpu_clk.hw,
>> > + .nb.notifier_call = meson_a1_cpu_clk_notifier_cb,
>> > +};
>> > +
>> > +static int meson_a1_dvfs_setup(struct platform_device *pdev)
>> > +{
>> > + struct device *dev = &pdev->dev;
>> > + struct clk *notifier_clk;
>> > + int ret;
>> > +
>> > + /* Setup clock notifier for cpu_clk */
>> > + notifier_clk = devm_clk_hw_get_clk(dev, &cpu_clk.hw, "dvfs");
>> > + if (IS_ERR(notifier_clk))
>> > + return dev_err_probe(dev, PTR_ERR(notifier_clk),
>> > + "can't get cpu_clk as notifier clock\n");
>> > +
>> > + ret = devm_clk_notifier_register(dev, notifier_clk,
>> > + &a1_cpu_clk_nb_data.nb);
>> > + if (ret)
>> > + return dev_err_probe(dev, ret,
>> > + "can't register cpu_clk notifier\n");
>> > +
>> > + return ret;
>> > +}
>> > +
>> > +static int meson_a1_cpu_probe(struct platform_device *pdev)
>> > +{
>> > + struct device *dev = &pdev->dev;
>> > + void __iomem *base;
>> > + struct regmap *map;
>> > + int clkid, i, err;
>> > +
>> > + base = devm_platform_ioremap_resource(pdev, 0);
>> > + if (IS_ERR(base))
>> > + return dev_err_probe(dev, PTR_ERR(base),
>> > + "can't ioremap resource\n");
>> > +
>> > + map = devm_regmap_init_mmio(dev, base, &a1_cpu_regmap_cfg);
>> > + if (IS_ERR(map))
>> > + return dev_err_probe(dev, PTR_ERR(map),
>> > + "can't init regmap mmio region\n");
>> > +
>> > + /* Populate regmap for the regmap backed clocks */
>> > + for (i = 0; i < ARRAY_SIZE(a1_cpu_regmaps); i++)
>> > + a1_cpu_regmaps[i]->map = map;
>> > +
>> > + for (clkid = 0; clkid < a1_cpu_clks.num; clkid++) {
>> > + err = devm_clk_hw_register(dev, a1_cpu_clks.hws[clkid]);
>> > + if (err)
>> > + return dev_err_probe(dev, err,
>> > + "clock[%d] registration failed\n",
>> > + clkid);
>> > + }
>> > +
>> > + err = devm_of_clk_add_hw_provider(dev, meson_clk_hw_get, &a1_cpu_clks);
>> > + if (err)
>> > + return dev_err_probe(dev, err, "can't add clk hw provider\n");
>>
>> I wonder if there is a window of opportunity to poke the syspll without
>> your notifier here. That being said, the situation would be similar on g12.
>>
>
> Yes, I have taken into account what you did in the G12A CPU clock
> relations. My thoughts were that it might not be applicable for the A1
> case. This is because the sys_pll should be located in a different
> driver from a logical perspective. Consequently, we cannot configure the
> sys_pll notifier block to manage the cpu_clk from a different driver.
> However, if I were to move the sys_pll clock object to the A1 CPU clock
> controller, I believe the g12a sys_pll notifier approach would work.
>
I fail to see the connection between the number of device and the
approach you took.
>> > +
>> > + return meson_a1_dvfs_setup(pdev);
>>
>>
>>
>> > +}
>> > +
>> > +static const struct of_device_id a1_cpu_clkc_match_table[] = {
>> > + { .compatible = "amlogic,a1-cpu-clkc", },
>> > + {}
>> > +};
>> > +MODULE_DEVICE_TABLE(of, a1_cpu_clkc_match_table);
>> > +
>> > +static struct platform_driver a1_cpu_clkc_driver = {
>> > + .probe = meson_a1_cpu_probe,
>> > + .driver = {
>> > + .name = "a1-cpu-clkc",
>> > + .of_match_table = a1_cpu_clkc_match_table,
>> > + },
>> > +};
>> > +
>> > +module_platform_driver(a1_cpu_clkc_driver);
>> > +MODULE_AUTHOR("Dmitry Rokosov <ddrokosov@salutedevices.com>");
>> > +MODULE_LICENSE("GPL");
>> > diff --git a/drivers/clk/meson/a1-cpu.h b/drivers/clk/meson/a1-cpu.h
>> > new file mode 100644
>> > index 000000000000..e9af4117e26f
>> > --- /dev/null
>> > +++ b/drivers/clk/meson/a1-cpu.h
>>
>> There is not point putting the definition here in a header
>> These are clearly not going to be shared with another driver.
>>
>> Please drop this file
>>
>
> The same approach was applied to the Peripherals and PLL A1 drivers.
> Honestly, I am not a fan of having different file organization within a
> single logical code folder.
>
> Please refer to:
>
> drivers/clk/meson/a1-peripherals.h
> drivers/clk/meson/a1-pll.h
I understand. There was a time where it made sense, some definition
could have been shared back then. This is no longer the case and it
would probably welcome a rework.
... and again, just pointing to another code does really invalidate my
point.
>
>> > @@ -0,0 +1,16 @@
>> > +/* SPDX-License-Identifier: GPL-2.0+ */
>> > +/*
>> > + * Amlogic A1 CPU Clock Controller internals
>> > + *
>> > + * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
>> > + * Author: Dmitry Rokosov <ddrokosov@salutedevices.com>
>> > + */
>> > +
>> > +#ifndef __A1_CPU_H
>> > +#define __A1_CPU_H
>> > +
>> > +/* cpu clock controller register offset */
>> > +#define CPUCTRL_CLK_CTRL0 0x80
>> > +#define CPUCTRL_CLK_CTRL1 0x84
>>
>> You are claiming the registers from 0x00 to 0x84 (included), but only
>> using these 2 registers ? What is the rest ? Are you sure there is only
>> clocks in there ?
>>
>
> Yes, unfortunately, the register map for this IP is not described in the
> A1 Datasheet. The only available information about it can be found in
> the vendor clock driver, which provides details for only two registers
> used to configure the CPU clock.
>
> From vendor kernel dtsi:
>
> clkc: clock-controller {
> compatible = "amlogic,a1-clkc";
> #clock-cells = <1>;
> reg = <0x0 0xfe000800 0x0 0x100>,
> <0x0 0xfe007c00 0x0 0x21c>,
> <0x0 0xfd000000 0x0 0x88>; <==== CPU clock regmap
> reg-names = "basic", "pll",
> "cpu_clk";
> clocks = <&xtal>;
> clock-names = "core";
> status = "okay";
> };
>
> From vendor clkc driver:
>
> /*
> * CPU clok register offset
> * APB_BASE: APB1_BASE_ADDR = 0xfd000000
> */
>
> #define CPUCTRL_CLK_CTRL0 0x80
> #define CPUCTRL_CLK_CTRL1 0x84
If you had clock in 0x0 and 0x80, then I would agree claiming 0x0-0x88
is reasonable, even if you don't really know what is in between. It
would be a fair assumption.
It is not the case here.
For all we know it could resets, power domains, etc ...
>
> [...]
--
Jerome
^ permalink raw reply
* Re: [PATCH 22/23] drivers: media: i2c: imx258: Add support for powerdown gpio
From: Dan Carpenter @ 2024-04-02 14:28 UTC (permalink / raw)
To: oe-kbuild, git, linux-media
Cc: lkp, oe-kbuild-all, dave.stevenson, jacopo.mondi, mchehab, robh,
krzysztof.kozlowski+dt, conor+dt, shawnguo, s.hauer, kernel,
festevam, sakari.ailus, devicetree, imx, linux-arm-kernel,
linux-kernel, Luigi311, Ondrej Jirman
In-Reply-To: <20240327231710.53188-23-git@luigi311.com>
Hi,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/git-luigi311-com/media-i2c-imx258-Remove-unused-defines/20240328-072629
base: git://linuxtv.org/media_tree.git master
patch link: https://lore.kernel.org/r/20240327231710.53188-23-git%40luigi311.com
patch subject: [PATCH 22/23] drivers: media: i2c: imx258: Add support for powerdown gpio
config: x86_64-randconfig-161-20240331 (https://download.01.org/0day-ci/archive/20240401/202404011425.PVKV9Lf1-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202404011425.PVKV9Lf1-lkp@intel.com/
smatch warnings:
drivers/media/i2c/imx258.c:1562 imx258_probe() warn: missing unwind goto?
vim +1562 drivers/media/i2c/imx258.c
d3773094af21c9 Dave Stevenson 2024-03-27 1476
e4802cb00bfe3d Jason Chen 2018-05-02 1477 static int imx258_probe(struct i2c_client *client)
e4802cb00bfe3d Jason Chen 2018-05-02 1478 {
e4802cb00bfe3d Jason Chen 2018-05-02 1479 struct imx258 *imx258;
786d2ad50b9b49 Dave Stevenson 2024-03-27 1480 struct fwnode_handle *endpoint;
786d2ad50b9b49 Dave Stevenson 2024-03-27 1481 struct v4l2_fwnode_endpoint ep = {
786d2ad50b9b49 Dave Stevenson 2024-03-27 1482 .bus_type = V4L2_MBUS_CSI2_DPHY
786d2ad50b9b49 Dave Stevenson 2024-03-27 1483 };
e4802cb00bfe3d Jason Chen 2018-05-02 1484 int ret;
e4802cb00bfe3d Jason Chen 2018-05-02 1485 u32 val = 0;
e4802cb00bfe3d Jason Chen 2018-05-02 1486
9fda25332c4b9e Krzysztof Kozlowski 2021-01-27 1487 imx258 = devm_kzalloc(&client->dev, sizeof(*imx258), GFP_KERNEL);
9fda25332c4b9e Krzysztof Kozlowski 2021-01-27 1488 if (!imx258)
9fda25332c4b9e Krzysztof Kozlowski 2021-01-27 1489 return -ENOMEM;
9fda25332c4b9e Krzysztof Kozlowski 2021-01-27 1490
d3773094af21c9 Dave Stevenson 2024-03-27 1491 ret = imx258_get_regulators(imx258, client);
d3773094af21c9 Dave Stevenson 2024-03-27 1492 if (ret)
d3773094af21c9 Dave Stevenson 2024-03-27 1493 return dev_err_probe(&client->dev, ret,
d3773094af21c9 Dave Stevenson 2024-03-27 1494 "failed to get regulators\n");
d3773094af21c9 Dave Stevenson 2024-03-27 1495
9fda25332c4b9e Krzysztof Kozlowski 2021-01-27 1496 imx258->clk = devm_clk_get_optional(&client->dev, NULL);
d170b0ea176098 Sakari Ailus 2021-08-16 1497 if (IS_ERR(imx258->clk))
d170b0ea176098 Sakari Ailus 2021-08-16 1498 return dev_err_probe(&client->dev, PTR_ERR(imx258->clk),
d170b0ea176098 Sakari Ailus 2021-08-16 1499 "error getting clock\n");
9fda25332c4b9e Krzysztof Kozlowski 2021-01-27 1500 if (!imx258->clk) {
9fda25332c4b9e Krzysztof Kozlowski 2021-01-27 1501 dev_dbg(&client->dev,
9fda25332c4b9e Krzysztof Kozlowski 2021-01-27 1502 "no clock provided, using clock-frequency property\n");
9fda25332c4b9e Krzysztof Kozlowski 2021-01-27 1503
e4802cb00bfe3d Jason Chen 2018-05-02 1504 device_property_read_u32(&client->dev, "clock-frequency", &val);
d170b0ea176098 Sakari Ailus 2021-08-16 1505 } else {
d170b0ea176098 Sakari Ailus 2021-08-16 1506 val = clk_get_rate(imx258->clk);
9fda25332c4b9e Krzysztof Kozlowski 2021-01-27 1507 }
8bde18cb296d0e Dave Stevenson 2024-03-27 1508
8bde18cb296d0e Dave Stevenson 2024-03-27 1509 switch (val) {
8bde18cb296d0e Dave Stevenson 2024-03-27 1510 case 19200000:
8bde18cb296d0e Dave Stevenson 2024-03-27 1511 imx258->link_freq_configs = link_freq_configs_19_2;
8bde18cb296d0e Dave Stevenson 2024-03-27 1512 imx258->link_freq_menu_items = link_freq_menu_items_19_2;
8bde18cb296d0e Dave Stevenson 2024-03-27 1513 break;
8bde18cb296d0e Dave Stevenson 2024-03-27 1514 case 24000000:
8bde18cb296d0e Dave Stevenson 2024-03-27 1515 imx258->link_freq_configs = link_freq_configs_24;
8bde18cb296d0e Dave Stevenson 2024-03-27 1516 imx258->link_freq_menu_items = link_freq_menu_items_24;
8bde18cb296d0e Dave Stevenson 2024-03-27 1517 break;
8bde18cb296d0e Dave Stevenson 2024-03-27 1518 default:
8bde18cb296d0e Dave Stevenson 2024-03-27 1519 dev_err(&client->dev, "input clock frequency of %u not supported\n",
8bde18cb296d0e Dave Stevenson 2024-03-27 1520 val);
e4802cb00bfe3d Jason Chen 2018-05-02 1521 return -EINVAL;
9fda25332c4b9e Krzysztof Kozlowski 2021-01-27 1522 }
e4802cb00bfe3d Jason Chen 2018-05-02 1523
786d2ad50b9b49 Dave Stevenson 2024-03-27 1524 endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL);
786d2ad50b9b49 Dave Stevenson 2024-03-27 1525 if (!endpoint) {
786d2ad50b9b49 Dave Stevenson 2024-03-27 1526 dev_err(&client->dev, "Endpoint node not found\n");
786d2ad50b9b49 Dave Stevenson 2024-03-27 1527 return -EINVAL;
786d2ad50b9b49 Dave Stevenson 2024-03-27 1528 }
786d2ad50b9b49 Dave Stevenson 2024-03-27 1529
786d2ad50b9b49 Dave Stevenson 2024-03-27 1530 ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep);
786d2ad50b9b49 Dave Stevenson 2024-03-27 1531 fwnode_handle_put(endpoint);
786d2ad50b9b49 Dave Stevenson 2024-03-27 1532 if (ret) {
786d2ad50b9b49 Dave Stevenson 2024-03-27 1533 dev_err(&client->dev, "Parsing endpoint node failed\n");
786d2ad50b9b49 Dave Stevenson 2024-03-27 1534 return ret;
786d2ad50b9b49 Dave Stevenson 2024-03-27 1535 }
786d2ad50b9b49 Dave Stevenson 2024-03-27 1536
786d2ad50b9b49 Dave Stevenson 2024-03-27 1537 /* Get number of data lanes */
a42d61a239fac8 Dave Stevenson 2024-03-27 1538 switch (ep.bus.mipi_csi2.num_data_lanes) {
a42d61a239fac8 Dave Stevenson 2024-03-27 1539 case 2:
a42d61a239fac8 Dave Stevenson 2024-03-27 1540 imx258->lane_mode_idx = IMX258_2_LANE_MODE;
a42d61a239fac8 Dave Stevenson 2024-03-27 1541 break;
a42d61a239fac8 Dave Stevenson 2024-03-27 1542 case 4:
a42d61a239fac8 Dave Stevenson 2024-03-27 1543 imx258->lane_mode_idx = IMX258_4_LANE_MODE;
a42d61a239fac8 Dave Stevenson 2024-03-27 1544 break;
a42d61a239fac8 Dave Stevenson 2024-03-27 1545 default:
786d2ad50b9b49 Dave Stevenson 2024-03-27 1546 dev_err(&client->dev, "Invalid data lanes: %u\n",
a42d61a239fac8 Dave Stevenson 2024-03-27 1547 ep.bus.mipi_csi2.num_data_lanes);
786d2ad50b9b49 Dave Stevenson 2024-03-27 1548 ret = -EINVAL;
786d2ad50b9b49 Dave Stevenson 2024-03-27 1549 goto error_endpoint_free;
786d2ad50b9b49 Dave Stevenson 2024-03-27 1550 }
786d2ad50b9b49 Dave Stevenson 2024-03-27 1551
7db096053387db Dave Stevenson 2024-03-27 1552 imx258->csi2_flags = ep.bus.mipi_csi2.flags;
7db096053387db Dave Stevenson 2024-03-27 1553
a8bb93eeccfa73 Dave Stevenson 2024-03-27 1554 imx258->variant_cfg = of_device_get_match_data(&client->dev);
a8bb93eeccfa73 Dave Stevenson 2024-03-27 1555 if (!imx258->variant_cfg)
a8bb93eeccfa73 Dave Stevenson 2024-03-27 1556 imx258->variant_cfg = &imx258_cfg;
a8bb93eeccfa73 Dave Stevenson 2024-03-27 1557
8a1906e91c0093 Luigi311 2024-03-27 1558 /* request optional power down pin */
8a1906e91c0093 Luigi311 2024-03-27 1559 imx258->powerdown_gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
8a1906e91c0093 Luigi311 2024-03-27 1560 GPIOD_OUT_HIGH);
8a1906e91c0093 Luigi311 2024-03-27 1561 if (IS_ERR(imx258->powerdown_gpio))
8a1906e91c0093 Luigi311 2024-03-27 @1562 return PTR_ERR(imx258->powerdown_gpio);
ret = PTR_ERR(imx258->powerdown_gpio);
goto error_endpoint_free;
8a1906e91c0093 Luigi311 2024-03-27 1563
e4802cb00bfe3d Jason Chen 2018-05-02 1564 /* Initialize subdev */
e4802cb00bfe3d Jason Chen 2018-05-02 1565 v4l2_i2c_subdev_init(&imx258->sd, client, &imx258_subdev_ops);
e4802cb00bfe3d Jason Chen 2018-05-02 1566
9fda25332c4b9e Krzysztof Kozlowski 2021-01-27 1567 /* Will be powered off via pm_runtime_idle */
9fda25332c4b9e Krzysztof Kozlowski 2021-01-27 1568 ret = imx258_power_on(&client->dev);
9fda25332c4b9e Krzysztof Kozlowski 2021-01-27 1569 if (ret)
786d2ad50b9b49 Dave Stevenson 2024-03-27 1570 goto error_endpoint_free;
9fda25332c4b9e Krzysztof Kozlowski 2021-01-27 1571
e4802cb00bfe3d Jason Chen 2018-05-02 1572 /* Check module identity */
e4802cb00bfe3d Jason Chen 2018-05-02 1573 ret = imx258_identify_module(imx258);
e4802cb00bfe3d Jason Chen 2018-05-02 1574 if (ret)
9fda25332c4b9e Krzysztof Kozlowski 2021-01-27 1575 goto error_identify;
e4802cb00bfe3d Jason Chen 2018-05-02 1576
e4802cb00bfe3d Jason Chen 2018-05-02 1577 /* Set default mode to max resolution */
e4802cb00bfe3d Jason Chen 2018-05-02 1578 imx258->cur_mode = &supported_modes[0];
e4802cb00bfe3d Jason Chen 2018-05-02 1579
e4802cb00bfe3d Jason Chen 2018-05-02 1580 ret = imx258_init_controls(imx258);
e4802cb00bfe3d Jason Chen 2018-05-02 1581 if (ret)
9fda25332c4b9e Krzysztof Kozlowski 2021-01-27 1582 goto error_identify;
e4802cb00bfe3d Jason Chen 2018-05-02 1583
e4802cb00bfe3d Jason Chen 2018-05-02 1584 /* Initialize subdev */
e4802cb00bfe3d Jason Chen 2018-05-02 1585 imx258->sd.internal_ops = &imx258_internal_ops;
e4802cb00bfe3d Jason Chen 2018-05-02 1586 imx258->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
e4802cb00bfe3d Jason Chen 2018-05-02 1587 imx258->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
e4802cb00bfe3d Jason Chen 2018-05-02 1588
e4802cb00bfe3d Jason Chen 2018-05-02 1589 /* Initialize source pad */
e4802cb00bfe3d Jason Chen 2018-05-02 1590 imx258->pad.flags = MEDIA_PAD_FL_SOURCE;
e4802cb00bfe3d Jason Chen 2018-05-02 1591
e4802cb00bfe3d Jason Chen 2018-05-02 1592 ret = media_entity_pads_init(&imx258->sd.entity, 1, &imx258->pad);
e4802cb00bfe3d Jason Chen 2018-05-02 1593 if (ret)
e4802cb00bfe3d Jason Chen 2018-05-02 1594 goto error_handler_free;
e4802cb00bfe3d Jason Chen 2018-05-02 1595
15786f7b564eff Sakari Ailus 2021-03-05 1596 ret = v4l2_async_register_subdev_sensor(&imx258->sd);
e4802cb00bfe3d Jason Chen 2018-05-02 1597 if (ret < 0)
e4802cb00bfe3d Jason Chen 2018-05-02 1598 goto error_media_entity;
e4802cb00bfe3d Jason Chen 2018-05-02 1599
e4802cb00bfe3d Jason Chen 2018-05-02 1600 pm_runtime_set_active(&client->dev);
e4802cb00bfe3d Jason Chen 2018-05-02 1601 pm_runtime_enable(&client->dev);
e4802cb00bfe3d Jason Chen 2018-05-02 1602 pm_runtime_idle(&client->dev);
786d2ad50b9b49 Dave Stevenson 2024-03-27 1603 v4l2_fwnode_endpoint_free(&ep);
e4802cb00bfe3d Jason Chen 2018-05-02 1604
e4802cb00bfe3d Jason Chen 2018-05-02 1605 return 0;
e4802cb00bfe3d Jason Chen 2018-05-02 1606
e4802cb00bfe3d Jason Chen 2018-05-02 1607 error_media_entity:
e4802cb00bfe3d Jason Chen 2018-05-02 1608 media_entity_cleanup(&imx258->sd.entity);
e4802cb00bfe3d Jason Chen 2018-05-02 1609
e4802cb00bfe3d Jason Chen 2018-05-02 1610 error_handler_free:
e4802cb00bfe3d Jason Chen 2018-05-02 1611 imx258_free_controls(imx258);
e4802cb00bfe3d Jason Chen 2018-05-02 1612
9fda25332c4b9e Krzysztof Kozlowski 2021-01-27 1613 error_identify:
9fda25332c4b9e Krzysztof Kozlowski 2021-01-27 1614 imx258_power_off(&client->dev);
9fda25332c4b9e Krzysztof Kozlowski 2021-01-27 1615
786d2ad50b9b49 Dave Stevenson 2024-03-27 1616 error_endpoint_free:
786d2ad50b9b49 Dave Stevenson 2024-03-27 1617 v4l2_fwnode_endpoint_free(&ep);
786d2ad50b9b49 Dave Stevenson 2024-03-27 1618
e4802cb00bfe3d Jason Chen 2018-05-02 1619 return ret;
e4802cb00bfe3d Jason Chen 2018-05-02 1620 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH 2/3] arm64: dts: qcom: msm8996: add glink-edge nodes
From: Konrad Dybcio @ 2024-04-02 14:31 UTC (permalink / raw)
To: Dmitry Baryshkov, Bjorn Andersson, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Mathieu Poirier, Sibi Sankar
Cc: linux-arm-msm, devicetree, linux-remoteproc, linux-kernel
In-Reply-To: <20240401-msm8996-remoteproc-v1-2-f02ab47fc728@linaro.org>
On 31.03.2024 11:10 PM, Dmitry Baryshkov wrote:
> MSM8996 provides limited glink support, so add corresponding device tree
> nodes. For example the following interfaces are provided on db820c:
>
> modem:
> 2080000.remoteproc:glink-edge.LOOPBACK_CTL_MPSS.-1.-1
> 2080000.remoteproc:glink-edge.glink_ssr.-1.-1
> 2080000.remoteproc:glink-edge.rpmsg_chrdev.0.0
>
> adsp:
> 9300000.remoteproc:glink-edge.LOOPBACK_CTL_LPASS.-1.-1
> 9300000.remoteproc:glink-edge.glink_ssr.-1.-1
> 9300000.remoteproc:glink-edge.rpmsg_chrdev.0.0
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Konrad
^ permalink raw reply
* Re: [PATCH v1 2/6] clk: meson: a1: pll: support 'syspll' general-purpose PLL for CPU clock
From: Jerome Brunet @ 2024-04-02 14:27 UTC (permalink / raw)
To: Dmitry Rokosov
Cc: Jerome Brunet, neil.armstrong, mturquette, sboyd, robh+dt,
krzysztof.kozlowski+dt, khilman, martin.blumenstingl, kernel,
rockosov, linux-amlogic, linux-clk, devicetree, linux-kernel,
linux-arm-kernel
In-Reply-To: <20240402121546.qrrc7r5un75464pb@CAB-WSD-L081021>
On Tue 02 Apr 2024 at 15:15, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
> On Tue, Apr 02, 2024 at 11:00:42AM +0200, Jerome Brunet wrote:
>>
>> On Fri 29 Mar 2024 at 23:58, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
>>
>> > The 'syspll' PLL, also known as the system PLL, is a general and
>> > essential PLL responsible for generating the CPU clock frequency.
>> > With its wide-ranging capabilities, it is designed to accommodate
>> > frequencies within the range of 768MHz to 1536MHz.
>> >
>> > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
>> > ---
>> > drivers/clk/meson/a1-pll.c | 78 ++++++++++++++++++++++++++++++++++++++
>> > drivers/clk/meson/a1-pll.h | 6 +++
>> > 2 files changed, 84 insertions(+)
>> >
>> > diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c
>> > index 60b2e53e7e51..02fd2d325cc6 100644
>> > --- a/drivers/clk/meson/a1-pll.c
>> > +++ b/drivers/clk/meson/a1-pll.c
>> > @@ -138,6 +138,81 @@ static struct clk_regmap hifi_pll = {
>> > },
>> > };
>> >
>> > +static const struct pll_mult_range sys_pll_mult_range = {
>> > + .min = 32,
>> > + .max = 64,
>> > +};
>> > +
>> > +/*
>> > + * We assume that the sys_pll_clk has already been set up by the low-level
>> > + * bootloaders as the main CPU PLL source. Therefore, it is not necessary to
>> > + * run the initialization sequence.
>> > + */
>>
>> I see no reason to make such assumption.
>> This clock is no read-only, it apparently is able to re-lock so assuming
>> anything from the bootloader is just asking from trouble
>>
>
> Indeed, I have implemented the following initialization sequence. I have
> dumped the bootloader setup and included it in the definition of my
> sys_pll. However, I have encountered an issue with the enable bit. If I
> leave the enable bit switched on by default, there is a possibility that
> the bootloader selects a fixed CPU clock while the sys_pll should be
> switched off. On the other hand, if I keep the enable bit switched off
> by default, the bootloader might configure the CPU clock to use sys_pll,
> resulting in the execution halting when the initialization sequence is
> run. This situation has led me to assume that we should place our trust
> in the bootloader setup.
>
> If you believe it is necessary to include the initialization sequence, I
> can prepare it with the sys_pll enabled by default.
I just noted your initial comment is misleading.
You could submit a patch to apply the init sequence only if the PLL is
not already enabled. Maybe even condition that to flag in the pll data
to avoid applying it to the other platforms for now.
>
>> > +static struct clk_regmap sys_pll = {
>> > + .data = &(struct meson_clk_pll_data){
>> > + .en = {
>> > + .reg_off = ANACTRL_SYSPLL_CTRL0,
>> > + .shift = 28,
>> > + .width = 1,
>> > + },
>> > + .m = {
>> > + .reg_off = ANACTRL_SYSPLL_CTRL0,
>> > + .shift = 0,
>> > + .width = 8,
>> > + },
>> > + .n = {
>> > + .reg_off = ANACTRL_SYSPLL_CTRL0,
>> > + .shift = 10,
>> > + .width = 5,
>> > + },
>> > + .frac = {
>> > + .reg_off = ANACTRL_SYSPLL_CTRL1,
>> > + .shift = 0,
>> > + .width = 19,
>> > + },
>> > + .l = {
>> > + .reg_off = ANACTRL_SYSPLL_STS,
>> > + .shift = 31,
>> > + .width = 1,
>> > + },
>> > + .current_en = {
>> > + .reg_off = ANACTRL_SYSPLL_CTRL0,
>> > + .shift = 26,
>> > + .width = 1,
>> > + },
>> > + .l_detect = {
>> > + .reg_off = ANACTRL_SYSPLL_CTRL2,
>> > + .shift = 6,
>> > + .width = 1,
>> > + },
>> > + .range = &sys_pll_mult_range,
>> > + },
>> > + .hw.init = &(struct clk_init_data){
>> > + .name = "sys_pll",
>> > + .ops = &meson_clk_pll_ops,
>> > + .parent_names = (const char *[]){ "syspll_in" },
>> > + .num_parents = 1,
>> > + /*
>> > + * This clock is used as the main CPU PLL source in low-level
>> > + * bootloaders, and it is necessary to mark it as critical.
>> > + */
>> > + .flags = CLK_IS_CRITICAL,
>>
>> No I don't think so. Downstream consumer maybe critical but that one is
>> not, unless it is read-only.
>>
>> A CPU pll, like on the g12 family, is unlikely to be read-only since the
>> PLL will need to relock to change rates. During this phase, there will
>> be no reate coming from the PLL so the PLL is not critical and you must
>> be able to "park" your CPU an another clock while poking this one
>>
>
> Initially, I tagged it with CLK_IS_CRITICAL because I observed in the
> kernel start that CCF disables it.
> However, upon further understanding,
> I realized that this happened due to other reasons. I believe that if I
> provide an init sequence where sys_pll is enabled by default, CCF will
> not disable this clock.
>
>> > + },
>> > +};
>> > +
>> > +static struct clk_fixed_factor sys_pll_div16 = {
>> > + .mult = 1,
>> > + .div = 16,
>> > + .hw.init = &(struct clk_init_data){
>> > + .name = "sys_pll_div16",
>> > + .ops = &clk_fixed_factor_ops,
>> > + .parent_hws = (const struct clk_hw *[]) {
>> > + &sys_pll.hw
>> > + },
>> > + .num_parents = 1,
>> > + },
>> > +};
>> > +
>> > static struct clk_fixed_factor fclk_div2_div = {
>> > .mult = 1,
>> > .div = 2,
>> > @@ -283,6 +358,8 @@ static struct clk_hw *a1_pll_hw_clks[] = {
>> > [CLKID_FCLK_DIV5] = &fclk_div5.hw,
>> > [CLKID_FCLK_DIV7] = &fclk_div7.hw,
>> > [CLKID_HIFI_PLL] = &hifi_pll.hw,
>> > + [CLKID_SYS_PLL] = &sys_pll.hw,
>> > + [CLKID_SYS_PLL_DIV16] = &sys_pll_div16.hw,
>> > };
>> >
>> > static struct clk_regmap *const a1_pll_regmaps[] = {
>> > @@ -293,6 +370,7 @@ static struct clk_regmap *const a1_pll_regmaps[] = {
>> > &fclk_div5,
>> > &fclk_div7,
>> > &hifi_pll,
>> > + &sys_pll,
>> > };
>> >
>> > static struct regmap_config a1_pll_regmap_cfg = {
>> > diff --git a/drivers/clk/meson/a1-pll.h b/drivers/clk/meson/a1-pll.h
>> > index 4be17b2bf383..666d9b2137e9 100644
>> > --- a/drivers/clk/meson/a1-pll.h
>> > +++ b/drivers/clk/meson/a1-pll.h
>> > @@ -18,6 +18,12 @@
>> > #define ANACTRL_FIXPLL_CTRL0 0x0
>> > #define ANACTRL_FIXPLL_CTRL1 0x4
>> > #define ANACTRL_FIXPLL_STS 0x14
>> > +#define ANACTRL_SYSPLL_CTRL0 0x80
>> > +#define ANACTRL_SYSPLL_CTRL1 0x84
>> > +#define ANACTRL_SYSPLL_CTRL2 0x88
>> > +#define ANACTRL_SYSPLL_CTRL3 0x8c
>> > +#define ANACTRL_SYSPLL_CTRL4 0x90
>> > +#define ANACTRL_SYSPLL_STS 0x94
>> > #define ANACTRL_HIFIPLL_CTRL0 0xc0
>> > #define ANACTRL_HIFIPLL_CTRL1 0xc4
>> > #define ANACTRL_HIFIPLL_CTRL2 0xc8
--
Jerome
^ permalink raw reply
* [PATCH v3 00/11] PCI: imx6: Fix\rename\clean up and add lut information for imx95
From: Frank Li @ 2024-04-02 14:33 UTC (permalink / raw)
To: Richard Zhu, Lucas Stach, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP Linux Team, Philipp Zabel, Liam Girdwood, Mark Brown,
Manivannan Sadhasivam, Krzysztof Kozlowski, Conor Dooley
Cc: linux-pci, imx, linux-arm-kernel, linux-kernel, bpf, devicetree,
Frank Li, Jason Liu
Fixed 8mp EP mode problem.
imx6 actaully for all imx chips (imx6*, imx7*, imx8*, imx9*). To avoid
confuse, rename all imx6_* to imx_*, IMX6_* to IMX_*. pci-imx6.c to
pci-imx.c to avoid confuse.
Using callback to reduce switch case for core reset and refclk.
Add imx95 iommux and its stream id information.
Base on linux-pci/controller/imx
To: Richard Zhu <hongxing.zhu@nxp.com>
To: Lucas Stach <l.stach@pengutronix.de>
To: Lorenzo Pieralisi <lpieralisi@kernel.org>
To: Krzysztof Wilczyński <kw@linux.com>
To: Rob Herring <robh@kernel.org>
To: Bjorn Helgaas <bhelgaas@google.com>
To: Shawn Guo <shawnguo@kernel.org>
To: Sascha Hauer <s.hauer@pengutronix.de>
To: Pengutronix Kernel Team <kernel@pengutronix.de>
To: Fabio Estevam <festevam@gmail.com>
To: NXP Linux Team <linux-imx@nxp.com>
To: Philipp Zabel <p.zabel@pengutronix.de>
To: Liam Girdwood <lgirdwood@gmail.com>
To: Mark Brown <broonie@kernel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
To: Conor Dooley <conor+dt@kernel.org>
Cc: linux-pci@vger.kernel.org
Cc: imx@lists.linux.dev
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: bpf@vger.kernel.org
Cc: devicetree@vger.kernel.org
Signed-off-by: Frank Li <Frank.Li@nxp.com>
Changes in v3:
- Add an EP fixed patch
PCI: imx6: Fix PCIe link down when i.MX8MM and i.MX8MP PCIe is EP mode
PCI: imx6: Fix i.MX8MP PCIe EP can not trigger MSI
- Add 8qxp rc support
dt-bing yaml pass binding check
make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- -j8 dt_binding_check DT_SCHEMA_FILES=fsl,imx6q-pcie.yaml
LINT Documentation/devicetree/bindings
DTEX Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.example.dts
CHKDT Documentation/devicetree/bindings/processed-schema.json
SCHEMA Documentation/devicetree/bindings/processed-schema.json
DTC_CHK Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.example.dtb
- Link to v2: https://lore.kernel.org/r/20240304-pci2_upstream-v2-0-ad07c5eb6d67@nxp.com
Changes in v2:
- remove file to 'pcie-imx.c'
- keep CONFIG unchange.
- Link to v1: https://lore.kernel.org/r/20240227-pci2_upstream-v1-0-b952f8333606@nxp.com
---
Frank Li (7):
PCI: imx6: Rename imx6_* with imx_*
PCI: imx6: Rename pci-imx6.c to pcie-imx.c
MAINTAINERS: pci: imx: update imx6* to imx* since rename driver file
PCI: imx: Simplify switch-case logic by involve set_ref_clk callback
PCI: imx: Simplify switch-case logic by involve core_reset callback
PCI: imx: Config look up table(LUT) to support MSI ITS and IOMMU for i.MX95
PCI: imx: Consolidate redundant if-checks
Richard Zhu (4):
PCI: imx6: Fix PCIe link down when i.MX8MM and i.MX8MP PCIe is EP mode
PCI: imx6: Fix i.MX8MP PCIe EP can not trigger MSI
dt-bindings: imx6q-pcie: Add i.MX8Q pcie compatible string
PCI: imx6: Add i.MX8Q PCIe support
.../bindings/pci/fsl,imx6q-pcie-common.yaml | 5 +
.../devicetree/bindings/pci/fsl,imx6q-pcie.yaml | 18 +
MAINTAINERS | 4 +-
drivers/pci/controller/dwc/Makefile | 2 +-
.../pci/controller/dwc/{pci-imx6.c => pcie-imx.c} | 1173 ++++++++++++--------
5 files changed, 727 insertions(+), 475 deletions(-)
---
base-commit: 2e45e73eebd43365cb585c49b3a671dcfae6b5b5
change-id: 20240227-pci2_upstream-0cdd19a15163
Best regards,
---
Frank Li <Frank.Li@nxp.com>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox