* [PATCH] ARM: dts: qcom: msm8974-sony-shinano: Enable vibrator
From: Luca Weiss @ 2024-04-06 15:27 UTC (permalink / raw)
To: ~postmarketos/upstreaming, phone-devel, Bjorn Andersson,
Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-arm-msm, devicetree, linux-kernel, Luca Weiss
Enable the vibrator connected to PM8941 found on the Sony shinano
platform.
Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
.../arm/boot/dts/qcom/qcom-msm8974pro-sony-xperia-shinano-common.dtsi | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm/boot/dts/qcom/qcom-msm8974pro-sony-xperia-shinano-common.dtsi b/arch/arm/boot/dts/qcom/qcom-msm8974pro-sony-xperia-shinano-common.dtsi
index 3a0c0035de09..e129bb1bd6ec 100644
--- a/arch/arm/boot/dts/qcom/qcom-msm8974pro-sony-xperia-shinano-common.dtsi
+++ b/arch/arm/boot/dts/qcom/qcom-msm8974pro-sony-xperia-shinano-common.dtsi
@@ -202,6 +202,10 @@ led@7 {
};
};
+&pm8941_vib {
+ status = "okay";
+};
+
&remoteproc_adsp {
cx-supply = <&pm8841_s2>;
status = "okay";
---
base-commit: 956abeb75f90eac3d5ba1f4cff7c048f7c079502
change-id: 20240406-shinano-vib-80e27e9bab2c
Best regards,
--
Luca Weiss <luca@z3ntu.xyz>
^ permalink raw reply related
* Re: [PATCH v5 0/2] iio: temperature: ltc2983: small improvements
From: Jonathan Cameron @ 2024-04-06 15:49 UTC (permalink / raw)
To: Nuno Sa
Cc: linux-iio, devicetree, Lars-Peter Clausen, Michael Hennerich,
Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Krzysztof Kozlowski
In-Reply-To: <20240404-ltc2983-misc-improv-v5-0-c1f58057fcea@analog.com>
On Thu, 4 Apr 2024 10:58:16 +0200
Nuno Sa <nuno.sa@analog.com> wrote:
> Hi Jonathan,
>
> As suggested by you, v5 only has the regulator stuff. I'll send another
> series for the new dev_errp_helper().
Applied.
Thanks,
Jonathan
>
> ---
> Changes in v5:
> - Dropped patches 1,2,5 and 6.
> - Link to v4: https://lore.kernel.org/all/20240328-ltc2983-misc-improv-v4-0-0cc428c07cd5@analog.com/
>
> ---
> Nuno Sa (2):
> dt-bindings: iio: temperature: ltc2983: document power supply
> iio: temperature: ltc2983: support vdd regulator
>
> Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml | 4 ++++
> drivers/iio/temperature/ltc2983.c | 5 +++++
> 2 files changed, 9 insertions(+)
> ---
> base-commit: 6020ca4de8e5404b20f15a6d9873cd6eb5f6d8d6
> change-id: 20240222-ltc2983-misc-improv-1c7a78ece93f
> --
>
> Thanks!
> - Nuno Sá
>
^ permalink raw reply
* Re: [PATCH] ARM: dts: qcom: msm8974-sony-shinano: Enable vibrator
From: Dmitry Baryshkov @ 2024-04-06 16:07 UTC (permalink / raw)
To: Luca Weiss
Cc: ~postmarketos/upstreaming, phone-devel, Bjorn Andersson,
Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20240406-shinano-vib-v1-1-fdd02af39d56@z3ntu.xyz>
On Sat, 6 Apr 2024 at 18:36, Luca Weiss <luca@z3ntu.xyz> wrote:
>
> Enable the vibrator connected to PM8941 found on the Sony shinano
> platform.
>
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
> .../arm/boot/dts/qcom/qcom-msm8974pro-sony-xperia-shinano-common.dtsi | 4 ++++
> 1 file changed, 4 insertions(+)
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v5 02/10] dt-bindings: mailbox: Add mboxes property for CMDQ secure driver
From: Jason-JH Lin (林睿祥) @ 2024-04-06 16:15 UTC (permalink / raw)
To: conor@kernel.org
Cc: linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
Houlong Wei (魏厚龙),
devicetree@vger.kernel.org, Shawn Sung (宋孝謙),
CK Hu (胡俊光), conor+dt@kernel.org,
robh@kernel.org, linux-arm-kernel@lists.infradead.org,
krzysztof.kozlowski+dt@linaro.org, matthias.bgg@gmail.com,
jassisinghbrar@gmail.com, angelogioacchino.delregno@collabora.com
In-Reply-To: <20240405-remindful-galley-2dee9eec4f34@spud>
On Fri, 2024-04-05 at 17:13 +0100, Conor Dooley wrote:
> On Fri, Apr 05, 2024 at 02:33:14PM +0000, Jason-JH Lin (林睿祥) wrote:
> > On Thu, 2024-04-04 at 15:52 +0100, Conor Dooley wrote:
> > > On Thu, Apr 04, 2024 at 04:31:06AM +0000, Jason-JH Lin (林睿祥)
> > > wrote:
> > > > Hi Conor,
> > > >
> > > > Thanks for the reviews.
> > > >
> > > > On Wed, 2024-04-03 at 16:46 +0100, Conor Dooley wrote:
> > > > > On Wed, Apr 03, 2024 at 06:25:54PM +0800, Shawn Sung wrote:
> > > > > > From: "Jason-JH.Lin" <jason-jh.lin@mediatek.com>
> > > > > >
> > > > > > Add mboxes to define a GCE loopping thread as a secure irq
> > > > > > handler.
> > > > > > This property is only required if CMDQ secure driver is
> > > > > > supported.
> > > > > >
> > > > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > > > > > Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com>
> > > > > > ---
> > > > > > .../bindings/mailbox/mediatek,gce-mailbox.yaml |
> > > > > > 10
> > > > > > ++++++++++
> > > > > > 1 file changed, 10 insertions(+)
> > > > > >
> > > > > > diff --git
> > > > > > a/Documentation/devicetree/bindings/mailbox/mediatek,gce-
> > > > > > mailbox.yaml
> > > > > > b/Documentation/devicetree/bindings/mailbox/mediatek,gce-
> > > > > > mailbox.yaml
> > > > > > index cef9d76013985..c0d80cc770899 100644
> > > > > > ---
> > > > > > a/Documentation/devicetree/bindings/mailbox/mediatek,gce-
> > > > > > mailbox.yaml
> > > > > > +++
> > > > > > b/Documentation/devicetree/bindings/mailbox/mediatek,gce-
> > > > > > mailbox.yaml
> > > > > > @@ -49,6 +49,16 @@ properties:
> > > > > > items:
> > > > > > - const: gce
> > > > > >
> > > > > > + mediatek,gce-events:
> > > > > > + description:
> > > > > > + The event id which is mapping to the specific
> > > > > > hardware
> > > > > > event
> > > > > > signal
> > > > > > + to gce. The event id is defined in the gce header
> > > > > > + include/dt-bindings/gce/<chip>-gce.h of each chips.
> > > > >
> > > > > Missing any info here about when this should be used, hint -
> > > > > you
> > > > > have
> > > > > it
> > > > > in the commit message.
> > > > >
> > > > > > + $ref: /schemas/types.yaml#/definitions/uint32-arrayi
> > > > >
> > > > > Why is the ID used by the CMDQ service not fixed for each
> > > > > SoC?
> > > > >
> > > >
> > > > I forgot to sync with Shawn about this:
> > > > https://lore.kernel.org/all/20240124011459.12204-1-jason-
> > > > jh.lin@mediatek.com
> > > >
> > > > I'll fix it at the next version.
> > >
> > > When I say "fixed" I don't mean "this is wrong, please fix it", I
> > > mean
> > > "why is the value not static for a particular SoC". This needs to
> > > be
> > > explained in the patch (and the description for the event here
> > > needs
> > > to
> > > explain what the gce-mailbox is reserving an event for).
> > >
> >
> > Oh, I see. Thanks for noticing me.
> >
> > We do want to reserve a static event ID for gce-mailbox to
> > different
> > SoCs. There are 2 mainly reasons to why we set it in DTS:
> > 1. There are 1024 events IDs for GCE to use to execute instructions
> > in
> > the specific event happened. These events could be signaled by HW
> > or SW
> > and their value would be different in different SoC because of HW
> > event
> > IDs distribution range from 0 to 1023.
> > If we set a static event ID: 855 for mt8188, it might be conflict
> > the
> > event ID original set in mt8195.
>
> That's not a problem, we have compatibles for this purpose.
I agree that compatibles can do the same things.
>
> > 2. If we defined the event ID in DTS, we might know how many SW or
> > HW
> > event IDs are used.
> > If someone wants to use a new event ID for a new feature, they
> > could
> > find out the used event IDs in DTS easily and avoid the event ID
> > conflicting.
>
> Are the event IDs not documented in the reference manual for the SoC
> in
> question? Or in documentation for the secure world for these devices?
> A
> DTS should not be the authoritive source for this information for
> developers.
>
The event IDs were defined in:
inculde/dt-bindings/mailbox/mediatek,mt8188-gce.h.
> Additionally, the driver could very easily detect if someone does
> happen
> to put in the reserved ID. That could be generically useful (IOW,
> check
> all of them for re-use) if the ID are to not allowed to be shared.
>
> > The reason why we define a event ID is we want to get a SW signal
> > from
> > secure world. We design a GCE looping thread in gce-mailbox driver
> > to
> > wait for the GCE execute done event for each cmdq secure packets
> > from
> > secure world.
>
> This sort of information needs to be in the commit message, but I
> don't
> think this property is needed at all since it seems to be something
> detectable from the compatible.
I think put this event ID in driver data and distinguish them by
different compatibles can achieve the same thing.
However, I originally thought that align to the existing way like
MUTEX, CCORR, WDMA in
https://lore.kernel.org/all/20240124011459.12204-4-jason-jh.lin@mediatek.com
would be better choice.
I think their usage of gce-events are the same.
What do you think?
Regards,
Jason-JH.Lin
^ permalink raw reply
* Re: [PATCH v2 00/11] iio: dac: support IIO backends on the output direction
From: Jonathan Cameron @ 2024-04-06 16:19 UTC (permalink / raw)
To: Nuno Sa
Cc: linux-iio, devicetree, Dragos Bogdan, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan, Paul Cercueil, Alexandru Ardelean
In-Reply-To: <20240405-iio-backend-axi-dac-v2-0-293bab7d5552@analog.com>
On Fri, 5 Apr 2024 16:59:58 +0200
Nuno Sa <nuno.sa@analog.com> wrote:
> Hi Jonathan,
>
> Here it goes version 2 of the output backend series. The main points in
> here:
> - The refactoring the DMA BUF api for setup. I pretty much like how it
> turned out. Note that Paul's patch ("iio: buffer-dmaengine: Support
> specifying buffer direction") had to be updated accordingly.
> - Introduction of the struct iio_info callback for getting the backend.
> I'm not sure about this one as we have no user for it and we may not
> have one for sometime. I like how the "default" implementation for
> getting the backend turned out and it should cover 99% of the cases. It
> will only fail if the iio parent device is not the same device where we
> bound the backend.
I've not looked at it yet, but this description makes me think perhaps
that should be ripped out for now? We can bring it back if we ever need it.
> - As mentioned above, we now get the backend from the iio device
> matching the IIO parent device with the device used when getting the
> backend. This should cover almost all the cases I think. Should be very
> unlikely to use a different device in devm_iio_backend_get() and
> devm_iio_device_alloc().
>
> For the bindings, I still did not addressed Rob's point about dma-names.
> I did reply [1] but still no feedback.
>
> Anyways, full log:
>
> v1:
> * https://lore.kernel.org/all/20240328-iio-backend-axi-dac-v1-0-afc808b3fde3@analog.com/
>
> v2:
> * Patch 1:
> - New patch.
>
> * Patch 4:
> - Make things consistent with the triggered buffer case.
>
> * Patch 6:
> - Fixed description as it's an output device;
> - Avoid duplicating the "bindings" word in the commit title.
>
> * Patch 7:
> - Renamed vdd_3_3-supply -> vdd-3p3-supply;
> - Added IRQ and vref properties;
> - Avoid duplicating the "bindings" word in the commit title.
>
> * Patch 8:
> - New patch.
>
> * Patch 9:
> - Fixed some typos in kerneldocs;
> - Add iio_backend_from_indio_dev_parent(). Default way of getting backends
> from IIO devices;
> - Explicitly differentiate frontends and backends ext_info in
> iio_backend_extend_chan_spec().
> - Spell out CW as CONTINUOUS_WAVE;
> - Add _hz suffix in set_sample_rate().
>
> * Patch 10:
> - Rephrase comment in axi_dac_set_sample_rate() when DDS is disabled;
> - Use the new iio_dmaengine_buffer_setup_ext() API;
> - Passed tone as 0,1 value being 1 second tone.
>
> * Patch 11:
> - Fixed mixed spaces with tabs in ABI file and dac -> DAC;
> - Add COMPILE_TEST to kconfig;
> - Dropped operating mode enum. Use defines;
> - Add comments for IIO enum operating mode and the value we need to
> set on the device;
> - Add spaces around {} in the reg_sequence;
> - Always use Mu instead of mixture of Mu and MU;
> - Don't error out if we do not recognize the part id;
> - Make sure to deal with other errors than TIMEOUT in ad9739a_init().
>
> [1]: https://lore.kernel.org/linux-iio/04e2a0569953792673319f7fcab3fe03e6670c03.camel@gmail.com/
>
> ---
> Nuno Sa (7):
> iio: buffer-dma: add iio_dmaengine_buffer_setup()
> dt-bindings: iio: dac: add docs for AXI DAC IP
> dt-bindings: iio: dac: add docs for AD9739A
> iio: core: add get_iio_backend() callback
> iio: backend: add new functionality
> iio: dac: add support for AXI DAC IP core
> iio: dac: support the ad9739a RF DAC
>
> Paul Cercueil (4):
> iio: buffer-dma: Rename iio_dma_buffer_data_available()
> iio: buffer-dma: Enable buffer write support
> iio: buffer-dmaengine: Support specifying buffer direction
> iio: buffer-dmaengine: Enable write support
>
> Documentation/ABI/testing/sysfs-bus-iio-ad9739a | 19 +
> .../devicetree/bindings/iio/dac/adi,ad9739a.yaml | 94 +++
> .../devicetree/bindings/iio/dac/adi,axi-dac.yaml | 62 ++
> MAINTAINERS | 17 +
> drivers/iio/adc/adi-axi-adc.c | 16 +-
> drivers/iio/buffer/industrialio-buffer-dma.c | 100 +++-
> drivers/iio/buffer/industrialio-buffer-dmaengine.c | 83 +--
> drivers/iio/dac/Kconfig | 37 ++
> drivers/iio/dac/Makefile | 2 +
> drivers/iio/dac/ad9739a.c | 454 +++++++++++++++
> drivers/iio/dac/adi-axi-dac.c | 635 +++++++++++++++++++++
> drivers/iio/industrialio-backend.c | 179 ++++++
> include/linux/iio/backend.h | 49 ++
> include/linux/iio/buffer-dma.h | 4 +-
> include/linux/iio/buffer-dmaengine.h | 24 +-
> include/linux/iio/iio.h | 2 +
> 16 files changed, 1698 insertions(+), 79 deletions(-)
> ---
> base-commit: 6020ca4de8e5404b20f15a6d9873cd6eb5f6d8d6
> change-id: 20240405-iio-backend-axi-dac-be99373b036b
> --
>
> Thanks!
> - Nuno Sá
>
^ permalink raw reply
* Re: [PATCH v2 03/11] iio: buffer-dma: Enable buffer write support
From: Jonathan Cameron @ 2024-04-06 16:23 UTC (permalink / raw)
To: Nuno Sa
Cc: linux-iio, devicetree, Dragos Bogdan, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan, Paul Cercueil, Alexandru Ardelean
In-Reply-To: <20240405-iio-backend-axi-dac-v2-3-293bab7d5552@analog.com>
On Fri, 5 Apr 2024 17:00:01 +0200
Nuno Sa <nuno.sa@analog.com> wrote:
> From: Paul Cercueil <paul@crapouillou.net>
>
> Adding write support to the buffer-dma code is easy - the write()
> function basically needs to do the exact same thing as the read()
> function: dequeue a block, read or write the data, enqueue the block
> when entirely processed.
>
> Therefore, the iio_buffer_dma_read() and the new iio_buffer_dma_write()
> now both call a function iio_buffer_dma_io(), which will perform this
> task.
>
> Note that we preemptively reset block->bytes_used to the buffer's size
> in iio_dma_buffer_request_update(), as in the future the
> iio_dma_buffer_enqueue() function won't reset it.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
One trivial comment on alignment that I noticed whilst reminding
myself of this patch. Otherwise looks good.
> +
> +/**
> + * iio_dma_buffer_read() - DMA buffer read callback
> + * @buffer: Buffer to read form
> + * @n: Number of bytes to read
> + * @user_buffer: Userspace buffer to copy the data to
> + *
> + * Should be used as the read callback for iio_buffer_access_ops
> + * struct for DMA buffers.
> + */
> +int iio_dma_buffer_read(struct iio_buffer *buffer, size_t n,
> + char __user *user_buffer)
Prefer aligning char with after the (
> +{
> + return iio_dma_buffer_io(buffer, n, user_buffer, false);
> +}
> EXPORT_SYMBOL_GPL(iio_dma_buffer_read);
>
> +/**
> + * iio_dma_buffer_write() - DMA buffer write callback
> + * @buffer: Buffer to read form
> + * @n: Number of bytes to read
> + * @user_buffer: Userspace buffer to copy the data from
> + *
> + * Should be used as the write callback for iio_buffer_access_ops
> + * struct for DMA buffers.
> + */
> +int iio_dma_buffer_write(struct iio_buffer *buffer, size_t n,
> + const char __user *user_buffer)
> +{
> + return iio_dma_buffer_io(buffer, n,
> + (__force __user char *)user_buffer, true);
> +}
> +EXPORT_SYMBOL_GPL(iio_dma_buffer_write);
>
^ permalink raw reply
* Re: [PATCH v2 09/11] iio: backend: add new functionality
From: Jonathan Cameron @ 2024-04-06 16:32 UTC (permalink / raw)
To: Nuno Sa
Cc: linux-iio, devicetree, Dragos Bogdan, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan
In-Reply-To: <20240405-iio-backend-axi-dac-v2-9-293bab7d5552@analog.com>
On Fri, 5 Apr 2024 17:00:07 +0200
Nuno Sa <nuno.sa@analog.com> wrote:
> This adds the needed backend ops for supporting a backend inerfacing
> with an high speed dac. The new ops are:
>
> * data_source_set();
> * set_sampling_freq();
> * extend_chan_spec();
> * ext_info_set();
> * ext_info_get().
>
> Also to note the new helpers that are meant to be used by the backends
> when extending an IIO channel (adding extended info):
>
> * iio_backend_ext_info_set();
> * iio_backend_ext_info_get().
>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
Whilst the code for the backend retrieval callback is simple
I wonder if we are better off just not having it for now.
Keep the infrastructure that checks for the default approach not working
but don't actually provide the alternative until we need it.
Advantage is pretty minor though so maybe just keep it.
Unless others have strong opinions, up to you to decide whether to keep it.
One trivial thing noticed inline.
> ---
> drivers/iio/industrialio-backend.c | 179 +++++++++++++++++++++++++++++++++++++
> include/linux/iio/backend.h | 49 ++++++++++
> 2 files changed, 228 insertions(+)
>
> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
> index 2fea2bbbe47f..ac554798897f 100644
> --- a/drivers/iio/industrialio-backend.c
> +++ b/drivers/iio/industrialio-backend.c
> @@ -29,6 +29,7 @@
> *
> * Copyright (C) 2023-2024 Analog Devices Inc.
> */
> +#include "asm-generic/errno-base.h"
You'll need a strong reason if you want to do that include rather than
a normal one like linux/errno.h
> #define dev_fmt(fmt) "iio-backend: " fmt
>
> #include <linux/cleanup.h>
> @@ -43,10 +44,12 @@
> #include <linux/types.h>
>
> #include <linux/iio/backend.h>
> +#include <linux/iio/iio.h>
>
^ permalink raw reply
* Re: [PATCH v2 11/11] iio: dac: support the ad9739a RF DAC
From: Jonathan Cameron @ 2024-04-06 16:41 UTC (permalink / raw)
To: Nuno Sa
Cc: linux-iio, devicetree, Dragos Bogdan, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan
In-Reply-To: <20240405-iio-backend-axi-dac-v2-11-293bab7d5552@analog.com>
On Fri, 5 Apr 2024 17:00:09 +0200
Nuno Sa <nuno.sa@analog.com> wrote:
> The AD9739A is a 14-bit, 2.5 GSPS high performance RF DACs that are capable
> of synthesizing wideband signals from DC up to 3 GHz.
>
> A dual-port, source synchronous, LVDS interface simplifies the digital
> interface with existing FGPA/ASIC technology. On-chip controllers are used
> to manage external and internal clock domain variations over temperature to
> ensure reliable data transfer from the host to the DAC core.
>
> Co-developed-by: Dragos Bogdan <dragos.bogdan@analog.com>
> Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
The only thing I really have remaining questions on is the choice
of chan_spec with altvoltage and voltage channels. Why does that
split make sense? It's odd enough that some comments in the code would
be a good thing to add.
Jonathan
> new file mode 100644
> index 000000000000..9b91d66f826c
> --- /dev/null
> +++ b/drivers/iio/dac/ad9739a.c
> @@ -0,0 +1,454 @@
> +
> +static struct iio_chan_spec ad9739a_channels[] = {
> + {
> + .type = IIO_ALTVOLTAGE,
So this looks a little unusual. Perhaps some comments on why it
is appropriate to have this channel.
In reality there is only one channel I think?
> + .indexed = 1,
> + .output = 1,
> + .scan_index = -1,
> + },
> + {
> + .type = IIO_VOLTAGE,
> + .indexed = 1,
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .output = 1,
> + .ext_info = ad9739a_ext_info,
> + .scan_type = {
> + .sign = 's',
> + .storagebits = 16,
> + .realbits = 16,
> + },
> + }
> +};
^ permalink raw reply
* Re: [PATCH 1/3] of: Add a helper to free property struct
From: Jonathan Cameron @ 2024-04-06 16:47 UTC (permalink / raw)
To: Rob Herring; +Cc: Saravana Kannan, devicetree, linux-kernel
In-Reply-To: <20240404-dt-cleanup-free-v1-1-c60e6cba8da9@kernel.org>
On Thu, 04 Apr 2024 09:15:10 -0500
Rob Herring <robh@kernel.org> wrote:
> Freeing a property struct is 3 kfree()'s which is duplicated in multiple
> spots. Add a helper, __of_prop_free(), and replace all the open coded
> cases in the DT code.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
^ permalink raw reply
* Re: [PATCH 2/3] of: Use scope based kfree() cleanups
From: Jonathan Cameron @ 2024-04-06 16:51 UTC (permalink / raw)
To: Rob Herring; +Cc: Saravana Kannan, devicetree, linux-kernel
In-Reply-To: <20240404-dt-cleanup-free-v1-2-c60e6cba8da9@kernel.org>
On Thu, 04 Apr 2024 09:15:11 -0500
Rob Herring <robh@kernel.org> wrote:
> Use the relatively new scope based kfree() cleanup to simplify error
> handling. Doing so reduces the chances of memory leaks and simplifies
> error paths by avoiding the need for goto statements.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
^ permalink raw reply
* Re: [PATCH 3/3] of: Use scope based of_node_put() cleanups
From: Jonathan Cameron @ 2024-04-06 17:17 UTC (permalink / raw)
To: Rob Herring; +Cc: Saravana Kannan, devicetree, linux-kernel
In-Reply-To: <20240404-dt-cleanup-free-v1-3-c60e6cba8da9@kernel.org>
On Thu, 04 Apr 2024 09:15:12 -0500
Rob Herring <robh@kernel.org> wrote:
> Use the relatively new scope based of_node_put() cleanup to simplify
> function exit handling. Doing so reduces the chances of forgetting an
> of_node_put() and simplifies error paths by avoiding the need for goto
> statements.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
Hi Rob,
Looks good in general. A few suggestions inline.
First one is a little more complex, but I think will give you a better end result
The others are readability improvements enabled by the changes you've made
that I think you could validly change in this same patch.
Jonathan
> ---
> drivers/of/address.c | 60 ++++++++++++++++-----------------------------------
> drivers/of/property.c | 22 ++++++-------------
> 2 files changed, 26 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index ae46a3605904..f7b2d535a6d1 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -491,7 +491,6 @@ static u64 __of_translate_address(struct device_node *dev,
> const __be32 *in_addr, const char *rprop,
> struct device_node **host)
> {
> - struct device_node *parent = NULL;
> struct of_bus *bus, *pbus;
> __be32 addr[OF_MAX_ADDR_CELLS];
> int na, ns, pna, pns;
> @@ -504,7 +503,7 @@ static u64 __of_translate_address(struct device_node *dev,
>
> *host = NULL;
> /* Get parent & match bus type */
> - parent = get_parent(dev);
> + struct device_node *parent __free(device_node) = get_parent(dev);
This is an order change as we'll put this node after dev
Whilst probably fine, I'd call that out in the patch description or just throw
in an earlier
struct device_node *dev_node __free(device_node) = of_node_get(dev);
Obviously the release order doesn't actually matter but a reader has to think
about it briefly to be sure.
Bonus if you do that is that you get to do direct returns at the error condition
and potentially in the breaks out of the loop. To my eye that's a readability
advantage.
In at least one place you could also steal the pointer rather than
getting another reference to dev (taking it to at least 2) then dropping
one of them in the exit path. Can use no_free_ptr() for that.
From readability point of view you'd only want to do that with a direct return.
*host = no_free_ptr(dev_node);
return result;
}
> if (parent == NULL)
> goto bail;
> bus = of_match_bus(parent);
> @@ -573,7 +572,6 @@ static u64 __of_translate_address(struct device_node *dev,
> of_dump_addr("one level translation:", addr, na);
> }
> bail:
> - of_node_put(parent);
> of_node_put(dev);
>
> @@ -1016,11 +999,9 @@ phys_addr_t __init of_dma_get_max_cpu_address(struct device_node *np)
> */
> bool of_dma_is_coherent(struct device_node *np)
> {
> - struct device_node *node;
> + struct device_node *node __free(device_node) = of_node_get(np);
> bool is_coherent = dma_default_coherent;
>
> - node = of_node_get(np);
> -
> while (node) {
> if (of_property_read_bool(node, "dma-coherent")) {
> is_coherent = true;
return true;
and same in the other branch given you break out the loop and return it directly.
> @@ -1032,7 +1013,6 @@ bool of_dma_is_coherent(struct device_node *np)
> }
> node = of_get_next_dma_parent(node);
> }
> - of_node_put(node);
> return is_coherent;
With above early returns
return dma_default_coherent;
and drop the is_coherent variable as no longer used.
This sort of related cleanup is one reason I really like the cleanup.h magic.
> }
> EXPORT_SYMBOL_GPL(of_dma_is_coherent);
> @@ -1049,19 +1029,17 @@ EXPORT_SYMBOL_GPL(of_dma_is_coherent);
> */
> static bool of_mmio_is_nonposted(struct device_node *np)
> {
> - struct device_node *parent;
> bool nonposted;
>
> if (!IS_ENABLED(CONFIG_ARCH_APPLE))
> return false;
>
> - parent = of_get_parent(np);
> + struct device_node *parent __free(device_node) = of_get_parent(np);
> if (!parent)
> return false;
>
> nonposted = of_property_read_bool(parent, "nonposted-mmio");
>
> - of_node_put(parent);
> return nonposted;
return of_property_read_bool()
> }
>
^ permalink raw reply
* Re: [PATCH v8 2/7] arm64: dts: qcom: sm8450: Add interconnect path to PCIe node
From: Krishna Chaitanya Chundru @ 2024-04-06 17:18 UTC (permalink / raw)
To: Manivannan Sadhasivam, Konrad Dybcio
Cc: Bjorn Andersson, Lorenzo Pieralisi, Krzysztof Wilczyński,
Rob Herring, Bjorn Helgaas, Krzysztof Kozlowski, Conor Dooley,
Rob Herring, Johan Hovold, Brian Masney, Georgi Djakov,
linux-arm-msm, linux-pci, devicetree, linux-kernel, vireshk,
quic_vbadigan, quic_skananth, quic_nitegupt, quic_parass
In-Reply-To: <20240405074044.GC2953@thinkpad>
On 4/5/2024 1:10 PM, Manivannan Sadhasivam wrote:
> On Wed, Mar 06, 2024 at 05:04:54PM +0100, Konrad Dybcio wrote:
>>
>>
>> On 3/2/24 04:59, Krishna chaitanya chundru wrote:
>>> Add pcie-mem & cpu-pcie interconnect path to the PCIe nodes.
>>>
>>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>>> ---
>>> arch/arm64/boot/dts/qcom/sm8450.dtsi | 8 ++++++++
>>> 1 file changed, 8 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>>> index 01e4dfc4babd..6b1d2e0d9d14 100644
>>> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>>> @@ -1781,6 +1781,10 @@ pcie0: pcie@1c00000 {
>>> <0 0 0 3 &intc 0 0 0 151 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
>>> <0 0 0 4 &intc 0 0 0 152 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
>>> + interconnects = <&pcie_noc MASTER_PCIE_0 0 &mc_virt SLAVE_EBI1 0>,
>>
>> Please use QCOM_ICC_TAG_ALWAYS.
>>
>>> + <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_PCIE_0 0>;
>>
>> And this path could presumably be demoted to QCOM_ICC_TAG_ACTIVE_ONLY?
>>
>
> I think it should be fine since there would be no register access done while the
> RPMh is put into sleep state. Krishna, can you confirm that by executing the CX
> shutdown with QCOM_ICC_TAG_ACTIVE_ONLY vote for cpu-pcie path on any supported
> platform?
>
> But if we do such change, then it should also be applied to other SoCs.
>
> - Mani
>
we don't a have platform to test this now, we will keep
QCOM_ICC_TAG_ALWAYS for now.
- Krishna Chaitanya.
^ permalink raw reply
* [PATCH] arm64: dts: rockchip: rk3588s: Fix ordering of nodes
From: Diederik de Haas @ 2024-04-06 17:28 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner
Cc: devicetree, linux-arm-kernel, linux-rockchip, Diederik de Haas
Fix the ordering of the main nodes by sorting them alphabetically and
then the ones with a memory address sequentially by that address.
Signed-off-by: Diederik de Haas <didi.debian@cknow.org>
---
arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 304 +++++++++++-----------
1 file changed, 152 insertions(+), 152 deletions(-)
diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
index 89d40cff635f..ac5bd630f155 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
@@ -347,6 +347,11 @@ l3_cache: l3-cache {
};
};
+ display_subsystem: display-subsystem {
+ compatible = "rockchip,display-subsystem";
+ ports = <&vop_out>;
+ };
+
firmware {
optee: optee {
compatible = "linaro,optee-tz";
@@ -394,11 +399,6 @@ spll: clock-0 {
#clock-cells = <0>;
};
- display_subsystem: display-subsystem {
- compatible = "rockchip,display-subsystem";
- ports = <&vop_out>;
- };
-
timer {
compatible = "arm,armv8-timer";
interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH 0>,
@@ -436,6 +436,62 @@ scmi_shmem: sram@0 {
};
};
+ gpu: gpu@fb000000 {
+ compatible = "rockchip,rk3588-mali", "arm,mali-valhall-csf";
+ reg = <0x0 0xfb000000 0x0 0x200000>;
+ #cooling-cells = <2>;
+ assigned-clocks = <&scmi_clk SCMI_CLK_GPU>;
+ assigned-clock-rates = <200000000>;
+ clocks = <&cru CLK_GPU>, <&cru CLK_GPU_COREGROUP>,
+ <&cru CLK_GPU_STACKS>;
+ clock-names = "core", "coregroup", "stacks";
+ dynamic-power-coefficient = <2982>;
+ interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH 0>,
+ <GIC_SPI 93 IRQ_TYPE_LEVEL_HIGH 0>,
+ <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH 0>;
+ interrupt-names = "job", "mmu", "gpu";
+ operating-points-v2 = <&gpu_opp_table>;
+ power-domains = <&power RK3588_PD_GPU>;
+ status = "disabled";
+
+ gpu_opp_table: opp-table {
+ compatible = "operating-points-v2";
+
+ opp-300000000 {
+ opp-hz = /bits/ 64 <300000000>;
+ opp-microvolt = <675000 675000 850000>;
+ };
+ opp-400000000 {
+ opp-hz = /bits/ 64 <400000000>;
+ opp-microvolt = <675000 675000 850000>;
+ };
+ opp-500000000 {
+ opp-hz = /bits/ 64 <500000000>;
+ opp-microvolt = <675000 675000 850000>;
+ };
+ opp-600000000 {
+ opp-hz = /bits/ 64 <600000000>;
+ opp-microvolt = <675000 675000 850000>;
+ };
+ opp-700000000 {
+ opp-hz = /bits/ 64 <700000000>;
+ opp-microvolt = <700000 700000 850000>;
+ };
+ opp-800000000 {
+ opp-hz = /bits/ 64 <800000000>;
+ opp-microvolt = <750000 750000 850000>;
+ };
+ opp-900000000 {
+ opp-hz = /bits/ 64 <900000000>;
+ opp-microvolt = <800000 800000 850000>;
+ };
+ opp-1000000000 {
+ opp-hz = /bits/ 64 <1000000000>;
+ opp-microvolt = <850000 850000 850000>;
+ };
+ };
+ };
+
usb_host0_ehci: usb@fc800000 {
compatible = "rockchip,rk3588-ehci", "generic-ehci";
reg = <0x0 0xfc800000 0x0 0x40000>;
@@ -501,62 +557,6 @@ usb_host2_xhci: usb@fcd00000 {
status = "disabled";
};
- gpu: gpu@fb000000 {
- compatible = "rockchip,rk3588-mali", "arm,mali-valhall-csf";
- reg = <0x0 0xfb000000 0x0 0x200000>;
- #cooling-cells = <2>;
- assigned-clocks = <&scmi_clk SCMI_CLK_GPU>;
- assigned-clock-rates = <200000000>;
- clocks = <&cru CLK_GPU>, <&cru CLK_GPU_COREGROUP>,
- <&cru CLK_GPU_STACKS>;
- clock-names = "core", "coregroup", "stacks";
- dynamic-power-coefficient = <2982>;
- interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH 0>,
- <GIC_SPI 93 IRQ_TYPE_LEVEL_HIGH 0>,
- <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH 0>;
- interrupt-names = "job", "mmu", "gpu";
- operating-points-v2 = <&gpu_opp_table>;
- power-domains = <&power RK3588_PD_GPU>;
- status = "disabled";
-
- gpu_opp_table: opp-table {
- compatible = "operating-points-v2";
-
- opp-300000000 {
- opp-hz = /bits/ 64 <300000000>;
- opp-microvolt = <675000 675000 850000>;
- };
- opp-400000000 {
- opp-hz = /bits/ 64 <400000000>;
- opp-microvolt = <675000 675000 850000>;
- };
- opp-500000000 {
- opp-hz = /bits/ 64 <500000000>;
- opp-microvolt = <675000 675000 850000>;
- };
- opp-600000000 {
- opp-hz = /bits/ 64 <600000000>;
- opp-microvolt = <675000 675000 850000>;
- };
- opp-700000000 {
- opp-hz = /bits/ 64 <700000000>;
- opp-microvolt = <700000 700000 850000>;
- };
- opp-800000000 {
- opp-hz = /bits/ 64 <800000000>;
- opp-microvolt = <750000 750000 850000>;
- };
- opp-900000000 {
- opp-hz = /bits/ 64 <900000000>;
- opp-microvolt = <800000 800000 850000>;
- };
- opp-1000000000 {
- opp-hz = /bits/ 64 <1000000000>;
- opp-microvolt = <850000 850000 850000>;
- };
- };
- };
-
pmu1grf: syscon@fd58a000 {
compatible = "rockchip,rk3588-pmugrf", "syscon", "simple-mfd";
reg = <0x0 0xfd58a000 0x0 0x10000>;
@@ -702,74 +702,6 @@ i2c0: i2c@fd880000 {
status = "disabled";
};
- vop: vop@fdd90000 {
- compatible = "rockchip,rk3588-vop";
- reg = <0x0 0xfdd90000 0x0 0x4200>, <0x0 0xfdd95000 0x0 0x1000>;
- reg-names = "vop", "gamma-lut";
- interrupts = <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH 0>;
- clocks = <&cru ACLK_VOP>,
- <&cru HCLK_VOP>,
- <&cru DCLK_VOP0>,
- <&cru DCLK_VOP1>,
- <&cru DCLK_VOP2>,
- <&cru DCLK_VOP3>,
- <&cru PCLK_VOP_ROOT>;
- clock-names = "aclk",
- "hclk",
- "dclk_vp0",
- "dclk_vp1",
- "dclk_vp2",
- "dclk_vp3",
- "pclk_vop";
- iommus = <&vop_mmu>;
- power-domains = <&power RK3588_PD_VOP>;
- rockchip,grf = <&sys_grf>;
- rockchip,vop-grf = <&vop_grf>;
- rockchip,vo1-grf = <&vo1_grf>;
- rockchip,pmu = <&pmu>;
- status = "disabled";
-
- vop_out: ports {
- #address-cells = <1>;
- #size-cells = <0>;
-
- vp0: port@0 {
- #address-cells = <1>;
- #size-cells = <0>;
- reg = <0>;
- };
-
- vp1: port@1 {
- #address-cells = <1>;
- #size-cells = <0>;
- reg = <1>;
- };
-
- vp2: port@2 {
- #address-cells = <1>;
- #size-cells = <0>;
- reg = <2>;
- };
-
- vp3: port@3 {
- #address-cells = <1>;
- #size-cells = <0>;
- reg = <3>;
- };
- };
- };
-
- vop_mmu: iommu@fdd97e00 {
- compatible = "rockchip,rk3588-iommu", "rockchip,rk3568-iommu";
- reg = <0x0 0xfdd97e00 0x0 0x100>, <0x0 0xfdd97f00 0x0 0x100>;
- interrupts = <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH 0>;
- clocks = <&cru ACLK_VOP>, <&cru HCLK_VOP>;
- clock-names = "aclk", "iface";
- #iommu-cells = <0>;
- power-domains = <&power RK3588_PD_VOP>;
- status = "disabled";
- };
-
uart0: serial@fd890000 {
compatible = "rockchip,rk3588-uart", "snps,dw-apb-uart";
reg = <0x0 0xfd890000 0x0 0x100>;
@@ -1140,6 +1072,87 @@ power-domain@RK3588_PD_SDMMC {
};
};
+ av1d: video-codec@fdc70000 {
+ compatible = "rockchip,rk3588-av1-vpu";
+ reg = <0x0 0xfdc70000 0x0 0x800>;
+ interrupts = <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH 0>;
+ interrupt-names = "vdpu";
+ assigned-clocks = <&cru ACLK_AV1>, <&cru PCLK_AV1>;
+ assigned-clock-rates = <400000000>, <400000000>;
+ clocks = <&cru ACLK_AV1>, <&cru PCLK_AV1>;
+ clock-names = "aclk", "hclk";
+ power-domains = <&power RK3588_PD_AV1>;
+ resets = <&cru SRST_A_AV1>, <&cru SRST_P_AV1>, <&cru SRST_A_AV1_BIU>, <&cru SRST_P_AV1_BIU>;
+ };
+
+ vop: vop@fdd90000 {
+ compatible = "rockchip,rk3588-vop";
+ reg = <0x0 0xfdd90000 0x0 0x4200>, <0x0 0xfdd95000 0x0 0x1000>;
+ reg-names = "vop", "gamma-lut";
+ interrupts = <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH 0>;
+ clocks = <&cru ACLK_VOP>,
+ <&cru HCLK_VOP>,
+ <&cru DCLK_VOP0>,
+ <&cru DCLK_VOP1>,
+ <&cru DCLK_VOP2>,
+ <&cru DCLK_VOP3>,
+ <&cru PCLK_VOP_ROOT>;
+ clock-names = "aclk",
+ "hclk",
+ "dclk_vp0",
+ "dclk_vp1",
+ "dclk_vp2",
+ "dclk_vp3",
+ "pclk_vop";
+ iommus = <&vop_mmu>;
+ power-domains = <&power RK3588_PD_VOP>;
+ rockchip,grf = <&sys_grf>;
+ rockchip,vop-grf = <&vop_grf>;
+ rockchip,vo1-grf = <&vo1_grf>;
+ rockchip,pmu = <&pmu>;
+ status = "disabled";
+
+ vop_out: ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ vp0: port@0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0>;
+ };
+
+ vp1: port@1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+ };
+
+ vp2: port@2 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <2>;
+ };
+
+ vp3: port@3 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <3>;
+ };
+ };
+ };
+
+ vop_mmu: iommu@fdd97e00 {
+ compatible = "rockchip,rk3588-iommu", "rockchip,rk3568-iommu";
+ reg = <0x0 0xfdd97e00 0x0 0x100>, <0x0 0xfdd97f00 0x0 0x100>;
+ interrupts = <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH 0>;
+ clocks = <&cru ACLK_VOP>, <&cru HCLK_VOP>;
+ clock-names = "aclk", "iface";
+ #iommu-cells = <0>;
+ power-domains = <&power RK3588_PD_VOP>;
+ status = "disabled";
+ };
+
i2s4_8ch: i2s@fddc0000 {
compatible = "rockchip,rk3588-i2s-tdm";
reg = <0x0 0xfddc0000 0x0 0x1000>;
@@ -1431,6 +1444,16 @@ qos_vop_m1: qos@fdf82200 {
reg = <0x0 0xfdf82200 0x0 0x20>;
};
+ dfi: dfi@fe060000 {
+ reg = <0x00 0xfe060000 0x00 0x10000>;
+ compatible = "rockchip,rk3588-dfi";
+ interrupts = <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH 0>,
+ <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH 0>,
+ <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH 0>,
+ <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH 0>;
+ rockchip,pmu = <&pmu1grf>;
+ };
+
pcie2x1l1: pcie@fe180000 {
compatible = "rockchip,rk3588-pcie", "rockchip,rk3568-pcie";
bus-range = <0x30 0x3f>;
@@ -1533,16 +1556,6 @@ pcie2x1l2_intc: legacy-interrupt-controller {
};
};
- dfi: dfi@fe060000 {
- reg = <0x00 0xfe060000 0x00 0x10000>;
- compatible = "rockchip,rk3588-dfi";
- interrupts = <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH 0>,
- <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH 0>,
- <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH 0>,
- <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH 0>;
- rockchip,pmu = <&pmu1grf>;
- };
-
gmac1: ethernet@fe1c0000 {
compatible = "rockchip,rk3588-gmac", "snps,dwmac-4.20a";
reg = <0x0 0xfe1c0000 0x0 0x10000>;
@@ -2543,19 +2556,6 @@ gpio4: gpio@fec50000 {
#interrupt-cells = <2>;
};
};
-
- av1d: video-codec@fdc70000 {
- compatible = "rockchip,rk3588-av1-vpu";
- reg = <0x0 0xfdc70000 0x0 0x800>;
- interrupts = <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH 0>;
- interrupt-names = "vdpu";
- assigned-clocks = <&cru ACLK_AV1>, <&cru PCLK_AV1>;
- assigned-clock-rates = <400000000>, <400000000>;
- clocks = <&cru ACLK_AV1>, <&cru PCLK_AV1>;
- clock-names = "aclk", "hclk";
- power-domains = <&power RK3588_PD_AV1>;
- resets = <&cru SRST_A_AV1>, <&cru SRST_P_AV1>, <&cru SRST_A_AV1_BIU>, <&cru SRST_P_AV1_BIU>;
- };
};
#include "rk3588s-pinctrl.dtsi"
base-commit: c1b1f340dd7db11f273e426e110697551c9f501f
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v2] dt-bindings: ti,pcm1681: Convert to dtschema
From: Krzysztof Kozlowski @ 2024-04-06 18:43 UTC (permalink / raw)
To: Animesh Agarwal
Cc: Shenghao Ding, Kevin Lu, Baojun Xu, Liam Girdwood, Mark Brown,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, alsa-devel,
devicetree, linux-kernel
In-Reply-To: <20240406141454.45529-1-animeshagarwal28@gmail.com>
On 06/04/2024 16:14, Animesh Agarwal wrote:
> Convert the Texas Instruments PCM1681 bindings to DT schema.
> Make bindings complete by adding #sound-dai-cells.
>
> Signed-off-by: Animesh Agarwal <animeshagarwal28@gmail.com>
>
> ---
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH net-next v6 11/17] dt-bindings: net: pse-pd: Add another way of describing several PSE PIs
From: Kory Maincent @ 2024-04-06 19:37 UTC (permalink / raw)
To: Rob Herring
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,
Maxime Chevallier
In-Reply-To: <20240404103854.29ef418c@kmaincent-XPS-13-7390>
On Thu, 4 Apr 2024 10:38:54 +0200
Kory Maincent <kory.maincent@bootlin.com> wrote:
> On Wed, 3 Apr 2024 09:31:42 -0500
> Rob Herring <robh@kernel.org> wrote:
>
> > On Wed, Apr 03, 2024 at 11:15:48AM +0200, Kory Maincent wrote:
> > > On Tue, 2 Apr 2024 08:26:37 -0500
> > > Rob Herring <robh@kernel.org> wrote:
> > >
> > > > > + pairset-names:
> > > > > + $ref: /schemas/types.yaml#/definitions/string-array
> > > > > + description:
> > > > > + Names of the pairsets as per IEEE 802.3-2022, Section
> > > > > 145.2.4.
> > > > > + Valid values are "alternative-a" and "alternative-b".
> > > > > Each name
> > > >
> > > > Don't state constraints in prose which are defined as schema
> > > > constraints.
> > >
> > > Ok, I will remove the line.
> > >
> > > > > + pairsets:
> > > > > + $ref: /schemas/types.yaml#/definitions/phandle-array
> > > > > + description:
> > > > > + List of phandles, each pointing to the power supply for
> > > > > the
> > > > > + corresponding pairset named in 'pairset-names'. This
> > > > > property
> > > > > + aligns with IEEE 802.3-2022, Section 33.2.3 and
> > > > > 145.2.4.
> > > > > + PSE Pinout Alternatives (as per IEEE 802.3-2022 Table
> > > > > 145\u20133)
> > > > > +
> > > > > |-----------|---------------|---------------|---------------|---------------|
> > > > > + | Conductor | Alternative A | Alternative A |
> > > > > Alternative B | Alternative B |
> > > > > + | | (MDI-X) | (MDI) | (X)
> > > > > | (S) |
> > > > > +
> > > > > |-----------|---------------|---------------|---------------|---------------|
> > > > > + | 1 | Negative VPSE | Positive VPSE | \u2014
> > > > > | \u2014 |
> > > > > + | 2 | Negative VPSE | Positive VPSE | \u2014
> > > > > | \u2014 |
> > > > > + | 3 | Positive VPSE | Negative VPSE | \u2014
> > > > > | \u2014 |
> > > > > + | 4 | \u2014 | \u2014 |
> > > > > Negative VPSE | Positive VPSE |
> > > > > + | 5 | \u2014 | \u2014 |
> > > > > Negative VPSE | Positive VPSE |
> > > > > + | 6 | Positive VPSE | Negative VPSE | \u2014
> > > > > | \u2014 |
> > > > > + | 7 | \u2014 | \u2014 |
> > > > > Positive VPSE | Negative VPSE |
> > > > > + | 8 | \u2014 | \u2014 |
> > > > > Positive VPSE | Negative VPSE |
> > > > > + minItems: 1
> > > > > + maxItems: 2
> > > >
> > > > "pairsets" does not follow the normal design pattern of foos,
> > > > foo-names, and #foo-cells. You could add #foo-cells I suppose, but what
> > > > would cells convey? I don't think it's a good fit for what you need.
> > > >
> > > > The other oddity is the number of entries and the names are fixed. That
> > > > is usually defined per consumer.
> > >
> > > Theoretically if the RJ45 port binding was supported it would make more
> > > sense, but in reality it's not feasible as the PSE controller need this
> > > information in its init process.
> > > The PSE controller reset all its port to apply a configuration so we can't
> > > do it when the consumer (RJ45) probe. It would reset the other ports if
> > > one consumer is probed later in the process.
> >
> > There is no reason other than convenience that all information some
> > driver needs has to be in one node or one hierarchy of nodes. You can
> > fetch anything from anywhere in the DT. It does feel like some of this
> > belongs in a connector node. We often haven't described connectors in DT
> > and stick connector properties in the controller node associated with
> > the connector. Then as things get more complicated, it becomes a mess.
>
> Right, we could indeed put all the informations of the pse_pi node in the
> future RJ45 port abstraction node. Then, this series will be put aside until
> we manage to have the port abstraction get merged.
> I am not glad about this as it will stuck my work until then, but indeed
> removing this pse_pi wrapper node which is between the pse_controller node and
> the connector node seems cleaner.
After some new thought, I thinks it is quite similar on the devicetree side to
have it in a pse_pi node or in the connector node.
Here are my agruments to continue using this pse_pi binding description:
- The connector abstraction is in its early work and won't really see a v1 soon
while the PoE series got mainly all reviewed-by thanks to Andrew.
This would stuck the PoE series until maybe one or two Linux version.
- It allows to use the "Power Interface" name like described in the standards.
- Even if this is in the PSE controller node, it is generic to all PSEs so it
shouldn't become a mess.
- It allows to have the PSE controller and Power Interfaces parameters grouped
together and it will be easier to read. May not really be an argument! ;)
- It will keep the logic of PoDL with the PHY using a single reference to the
PSE PI through the pses parameter.
Is it okay for you to continue with it?
Regards,
--
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
^ permalink raw reply
* RE: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set additionalProperties to true
From: Peng Fan @ 2024-04-07 0:37 UTC (permalink / raw)
To: Krzysztof Kozlowski, Peng Fan (OSS), Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Sudeep Holla,
Cristian Marussi
Cc: devicetree@vger.kernel.org, imx@lists.linux.dev,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <614b5107-656d-4d41-99c1-77941c48342c@kernel.org>
> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> additionalProperties to true
>
> On 05/04/2024 14:39, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > When adding vendor extension protocols, there is dt-schema warning:
> > "
> > imx,scmi.example.dtb: scmi: 'protocol@81', 'protocol@84' do not match
> > any of the regexes: 'pinctrl-[0-9]+'
> > "
> >
> > Set additionalProperties to true to address the issue.
>
> I do not see anything addressed here, except making the binding accepting
> anything anywhere...
I not wanna add vendor protocols in arm,scmi.yaml, so will introduce
a new yaml imx.scmi.yaml which add i.MX SCMI protocol extension.
With additionalProperties set to false, I not know how, please suggest.
Thanks,
Peng.
>
> Best regards,
> Krzysztof
^ permalink raw reply
* RE: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI Extension protocol
From: Peng Fan @ 2024-04-07 0:51 UTC (permalink / raw)
To: Krzysztof Kozlowski, Peng Fan (OSS), Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Sudeep Holla,
Cristian Marussi
Cc: devicetree@vger.kernel.org, imx@lists.linux.dev,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <9aab8dab-27c4-40b6-b1d5-0a2babe0700d@kernel.org>
> Subject: Re: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI Extension
> protocol
>
> On 05/04/2024 14:39, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Add i.MX SCMI Extension protocols bindings for:
> > - Battery Backed Secure Module(BBSM)
>
> Which is what?
I should say BBM(BBSM + BBNSM), BBM has RTC and ON/OFF
key features, but BBM is managed by SCMI firmware and exported
to agent by BBM protocol. So add bindings for i.MX BBM protocol.
Is this ok?
>
> > - MISC settings such as General Purpose Registers settings.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> > .../devicetree/bindings/firmware/imx,scmi.yaml | 80
> ++++++++++++++++++++++
> > 1 file changed, 80 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> > b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> > new file mode 100644
> > index 000000000000..7ee19a661d83
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> > @@ -0,0 +1,80 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) # Copyright 2024
> > +NXP %YAML 1.2
> > +---
> > +$id:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fschemas%2Ffirmware%2Fimx%2Cscmi.yaml%23&data=05%7
> C02%7Cp
> >
> +eng.fan%40nxp.com%7C5d16781d3eca425a342508dc562910b7%7C686ea
> 1d3bc2b4c
> >
> +6fa92cd99c5c301635%7C0%7C0%7C638479981570959816%7CUnknown%
> 7CTWFpbGZsb
> >
> +3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
> 0%3D
> >
> +%7C0%7C%7C%7C&sdata=mWNwPvu2eyF18MroVOBHb%2Fjeo%2BIHfV5V
> h%2F9ebdx65MM
> > +%3D&reserved=0
> > +$schema:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > +cetree.org%2Fmeta-
> schemas%2Fcore.yaml%23&data=05%7C02%7Cpeng.fan%40nx
> >
> +p.com%7C5d16781d3eca425a342508dc562910b7%7C686ea1d3bc2b4c6fa
> 92cd99c5c
> >
> +301635%7C0%7C0%7C638479981570971949%7CUnknown%7CTWFpbGZs
> b3d8eyJWIjoiM
> >
> +C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7
> C%7C%7
> >
> +C&sdata=v4XnGG00D4I8j5MJvDUVYMRTm7yRrvz0V3fUyc5KAAA%3D&reser
> ved=0
> > +
> > +title: i.MX System Control and Management Interface(SCMI) Vendor
> > +Protocols Extension
> > +
> > +maintainers:
> > + - Peng Fan <peng.fan@nxp.com>
> > +
> > +allOf:
> > + - $ref: arm,scmi.yaml#
>
> Sorry, but arm,scmi is a final schema. Is your plan to define some common
> part?
No. I just wanna add vendor extension per SCMI spec.
0x80-0xFF:
Reserved for vendor or platform-specific extensions to this interface
Each vendor may have different usage saying id 0x81, so I add
i.MX dt-schema file.
>
> > +
> > +properties:
> > + protocol@81:
> > + $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> > + unevaluatedProperties: false
> > + description:
> > + The BBM Protocol is for managing Battery Backed Secure Module
> (BBSM) RTC
> > + and the ON/OFF Key
> > +
> > + properties:
> > + reg:
> > + const: 0x81
> > +
> > + required:
> > + - reg
> > +
> > + protocol@84:
> > + $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> > + unevaluatedProperties: false
> > + description:
> > + The MISC Protocol is for managing SoC Misc settings, such as
> > + GPR settings
>
> Genera register is not a setting... this is a pleonasm. Please be more specific
> what is the GPR, MISC protocol etc.
The MISC Protocol is for managing SoC Misc settings, such as SAI MCLK/MQS in
Always On domain BLK CTRL, SAI_CLK_SEL in WAKEUP BLK CTRL, gpio
expanders which is under control of SCMI firmware.
> > +
> > + properties:
> > + reg:
> > + const: 0x84
> > +
> > + wakeup-sources:
> > + description:
> > + Each entry consists of 2 integers, represents the source
> > + and electric signal edge
>
> Can you answer questions from reviewers?
Sorry. Is this ok?
minItems: 1
maxItems: 32
>
> > + items:
> > + items:
> > + - description: the wakeup source
> > + - description: the wakeup electric signal edge
> > + $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > +
> > + required:
> > + - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + firmware {
> > + scmi {
> > + compatible = "arm,scmi";
>
> > + mboxes = <&mu2 5 0>, <&mu2 3 0>, <&mu2 3 1>;
> > + shmem = <&scmi_buf0>, <&scmi_buf1>;
> > +
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + protocol@81 {
> > + reg = <0x81>;
> > + };
> > +
> > + protocol@84 {
> > + reg = <0x84>;
> > + wakeup-sources = <0x8000 1
> > + 0x8001 1
> > + 0x8002 1
> > + 0x8003 1
> > + 0x8004 1>;
>
> Nothing improved... If you are going to ignore reviews, then you will only get
> NAKed.
Sorry, you mean the examples, or the whole dt-schema?
Thanks,
Peng.
>
> Best regards,
> Krzysztof
^ permalink raw reply
* RE: [PATCH v2 4/6] firmware: arm_scmi: add initial support for i.MX MISC protocol
From: Peng Fan @ 2024-04-07 1:03 UTC (permalink / raw)
To: Marco Felsch, Peng Fan (OSS)
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Sudeep Holla, Cristian Marussi, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, imx@lists.linux.dev
In-Reply-To: <20240405164414.t3fqhnwudxxt2ozv@pengutronix.de>
> Subject: Re: [PATCH v2 4/6] firmware: arm_scmi: add initial support for i.MX
> MISC protocol
>
> Hi Peng,
>
> On 24-04-05, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > The i.MX MISC protocol is for misc settings, such as gpio expander
> > wakeup.
>
> Can you elaborate a bit more please?
The gpio expander is under M33(SCMI firmware used core) I2C control,
But the gpio expander supports board function such as PCIE_WAKEUP,
BTN_WAKEUP. So these are managed by MISC protocol.
SAI_CLK_MSEL in WAKEUP BLK CTRL is also managed by MISC Protocol.
And etc...
I will add more info in commit log in next version later, after I get more
reviews on the patchset.
Thanks,
Peng.
>
> Regards,
> Marco
>
>
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> > drivers/firmware/arm_scmi/Kconfig | 10 ++
> > drivers/firmware/arm_scmi/Makefile | 1 +
> > drivers/firmware/arm_scmi/imx-sm-misc.c | 305
> ++++++++++++++++++++++++++++++++
> > include/linux/scmi_imx_protocol.h | 17 ++
> > 4 files changed, 333 insertions(+)
> >
> > diff --git a/drivers/firmware/arm_scmi/Kconfig
> > b/drivers/firmware/arm_scmi/Kconfig
> > index 56d11c9d9f47..bfeae92f6420 100644
> > --- a/drivers/firmware/arm_scmi/Kconfig
> > +++ b/drivers/firmware/arm_scmi/Kconfig
> > @@ -191,3 +191,13 @@ config IMX_SCMI_BBM_EXT
> > and BUTTON.
> >
> > This driver can also be built as a module.
> > +
> > +config IMX_SCMI_MISC_EXT
> > + tristate "i.MX SCMI MISC EXTENSION"
> > + depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
> > + default y if ARCH_MXC
> > + help
> > + This enables i.MX System MISC control logic such as gpio expander
> > + wakeup
> > +
> > + This driver can also be built as a module.
> > diff --git a/drivers/firmware/arm_scmi/Makefile
> > b/drivers/firmware/arm_scmi/Makefile
> > index 327687acf857..a23fde721222 100644
> > --- a/drivers/firmware/arm_scmi/Makefile
> > +++ b/drivers/firmware/arm_scmi/Makefile
> > @@ -12,6 +12,7 @@ scmi-transport-
> $(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO)
> > += virtio.o
> > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> > scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o
> > system.o voltage.o powercap.o
> > scmi-protocols-$(CONFIG_IMX_SCMI_BBM_EXT) += imx-sm-bbm.o
> > +scmi-protocols-$(CONFIG_IMX_SCMI_MISC_EXT) += imx-sm-misc.o
> > scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y)
> > $(scmi-transport-y)
> >
> > obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o diff --git
> > a/drivers/firmware/arm_scmi/imx-sm-misc.c
> > b/drivers/firmware/arm_scmi/imx-sm-misc.c
> > new file mode 100644
> > index 000000000000..1b0ec2281518
> > --- /dev/null
> > +++ b/drivers/firmware/arm_scmi/imx-sm-misc.c
> > @@ -0,0 +1,305 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * System control and Management Interface (SCMI) NXP MISC Protocol
> > + *
> > + * Copyright 2024 NXP
> > + */
> > +
> > +#define pr_fmt(fmt) "SCMI Notifications MISC - " fmt
> > +
> > +#include <linux/bits.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/scmi_protocol.h>
> > +#include <linux/scmi_imx_protocol.h>
> > +
> > +#include "protocols.h"
> > +#include "notify.h"
> > +
> > +#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x10000
> > +
> > +enum scmi_imx_misc_protocol_cmd {
> > + SCMI_IMX_MISC_CTRL_SET = 0x3,
> > + SCMI_IMX_MISC_CTRL_GET = 0x4,
> > + SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
> > +};
> > +
> > +struct scmi_imx_misc_info {
> > + u32 version;
> > + u32 nr_dev_ctrl;
> > + u32 nr_brd_ctrl;
> > + u32 nr_reason;
> > +};
> > +
> > +struct scmi_msg_imx_misc_protocol_attributes {
> > + __le32 attributes;
> > +};
> > +
> > +#define GET_BRD_CTRLS_NR(x) le32_get_bits((x), GENMASK(31,
> 24))
> > +#define GET_REASONS_NR(x) le32_get_bits((x), GENMASK(23, 16))
> > +#define GET_DEV_CTRLS_NR(x) le32_get_bits((x), GENMASK(15, 0))
> > +#define BRD_CTRL_START_ID BIT(15)
> > +
> > +struct scmi_imx_misc_ctrl_set_in {
> > + __le32 id;
> > + __le32 num;
> > + __le32 value[MISC_MAX_VAL];
> > +};
> > +
> > +struct scmi_imx_misc_ctrl_notify_in {
> > + __le32 ctrl_id;
> > + __le32 flags;
> > +};
> > +
> > +struct scmi_imx_misc_ctrl_notify_payld {
> > + __le32 ctrl_id;
> > + __le32 flags;
> > +};
> > +
> > +struct scmi_imx_misc_ctrl_get_out {
> > + __le32 num;
> > + __le32 *val;
> > +};
> > +
> > +static int scmi_imx_misc_attributes_get(const struct scmi_protocol_handle
> *ph,
> > + struct scmi_imx_misc_info *mi)
> > +{
> > + int ret;
> > + struct scmi_xfer *t;
> > + struct scmi_msg_imx_misc_protocol_attributes *attr;
> > +
> > + ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0,
> > + sizeof(*attr), &t);
> > + if (ret)
> > + return ret;
> > +
> > + attr = t->rx.buf;
> > +
> > + ret = ph->xops->do_xfer(ph, t);
> > + if (!ret) {
> > + mi->nr_dev_ctrl = GET_DEV_CTRLS_NR(attr->attributes);
> > + mi->nr_brd_ctrl = GET_BRD_CTRLS_NR(attr->attributes);
> > + mi->nr_reason = GET_REASONS_NR(attr->attributes);
> > + dev_info(ph->dev, "i.MX MISC NUM DEV CTRL: %d, NUM
> BRD CTRL: %d,NUM Reason: %d\n",
> > + mi->nr_dev_ctrl, mi->nr_brd_ctrl, mi->nr_reason);
> > + }
> > +
> > + ph->xops->xfer_put(ph, t);
> > +
> > + return ret;
> > +}
> > +
> > +static int scmi_imx_misc_ctrl_validate_id(const struct
> scmi_protocol_handle *ph,
> > + u32 ctrl_id)
> > +{
> > + struct scmi_imx_misc_info *mi = ph->get_priv(ph);
> > +
> > + if ((ctrl_id < BRD_CTRL_START_ID) && (ctrl_id > mi->nr_dev_ctrl))
> > + return -EINVAL;
> > + if (ctrl_id >= BRD_CTRL_START_ID + mi->nr_brd_ctrl)
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static int scmi_imx_misc_ctrl_notify(const struct scmi_protocol_handle
> *ph,
> > + u32 ctrl_id, u32 flags)
> > +{
> > + struct scmi_imx_misc_ctrl_notify_in *in;
> > + struct scmi_xfer *t;
> > + int ret;
> > +
> > + ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_NOTIFY,
> > + sizeof(*in), 0, &t);
> > + if (ret)
> > + return ret;
> > +
> > + in = t->tx.buf;
> > + in->ctrl_id = cpu_to_le32(ctrl_id);
> > + in->flags = cpu_to_le32(flags);
> > +
> > + ret = ph->xops->do_xfer(ph, t);
> > +
> > + ph->xops->xfer_put(ph, t);
> > +
> > + return ret;
> > +}
> > +
> > +static int
> > +scmi_imx_misc_ctrl_set_notify_enabled(const struct
> scmi_protocol_handle *ph,
> > + u8 evt_id, u32 src_id, bool enable) {
> > + int ret;
> > +
> > + ret = scmi_imx_misc_ctrl_notify(ph, src_id, enable ? evt_id : 0);
> > + if (ret)
> > + dev_err(ph->dev, "FAIL_ENABLED - evt[%X] src[%d] -
> ret:%d\n",
> > + evt_id, src_id, ret);
> > +
> > + return ret;
> > +}
> > +
> > +static int scmi_imx_misc_ctrl_get_num_sources(const struct
> > +scmi_protocol_handle *ph) {
> > + return GENMASK(15, 0);
> > +}
> > +
> > +static void *
> > +scmi_imx_misc_ctrl_fill_custom_report(const struct scmi_protocol_handle
> *ph,
> > + u8 evt_id, ktime_t timestamp,
> > + const void *payld, size_t payld_sz,
> > + void *report, u32 *src_id)
> > +{
> > + const struct scmi_imx_misc_ctrl_notify_payld *p = payld;
> > + struct scmi_imx_misc_ctrl_notify_report *r = report;
> > +
> > + if (sizeof(*p) != payld_sz)
> > + return NULL;
> > +
> > + r->timestamp = timestamp;
> > + r->ctrl_id = p->ctrl_id;
> > + r->flags = p->flags;
> > + *src_id = r->ctrl_id;
> > + dev_dbg(ph->dev, "%s: ctrl_id: %d flags: %d\n", __func__,
> > + r->ctrl_id, r->flags);
> > +
> > + return r;
> > +}
> > +
> > +static const struct scmi_event_ops scmi_imx_misc_event_ops = {
> > + .get_num_sources = scmi_imx_misc_ctrl_get_num_sources,
> > + .set_notify_enabled = scmi_imx_misc_ctrl_set_notify_enabled,
> > + .fill_custom_report = scmi_imx_misc_ctrl_fill_custom_report,
> > +};
> > +
> > +static const struct scmi_event scmi_imx_misc_events[] = {
> > + {
> > + .id = SCMI_EVENT_IMX_MISC_CONTROL_DISABLED,
> > + .max_payld_sz = sizeof(struct
> scmi_imx_misc_ctrl_notify_payld),
> > + .max_report_sz = sizeof(struct
> scmi_imx_misc_ctrl_notify_report),
> > + },
> > + {
> > + .id = SCMI_EVENT_IMX_MISC_CONTROL_FALLING_EDGE,
> > + .max_payld_sz = sizeof(struct
> scmi_imx_misc_ctrl_notify_payld),
> > + .max_report_sz = sizeof(struct
> scmi_imx_misc_ctrl_notify_report),
> > + },
> > + {
> > + .id = SCMI_EVENT_IMX_MISC_CONTROL_RISING_EDGE,
> > + .max_payld_sz = sizeof(struct
> scmi_imx_misc_ctrl_notify_payld),
> > + .max_report_sz = sizeof(struct
> scmi_imx_misc_ctrl_notify_report),
> > + }
> > +};
> > +
> > +static struct scmi_protocol_events scmi_imx_misc_protocol_events = {
> > + .queue_sz = SCMI_PROTO_QUEUE_SZ,
> > + .ops = &scmi_imx_misc_event_ops,
> > + .evts = scmi_imx_misc_events,
> > + .num_events = ARRAY_SIZE(scmi_imx_misc_events), };
> > +
> > +static int scmi_imx_misc_protocol_init(const struct
> > +scmi_protocol_handle *ph) {
> > + struct scmi_imx_misc_info *minfo;
> > + u32 version;
> > + int ret;
> > +
> > + ret = ph->xops->version_get(ph, &version);
> > + if (ret)
> > + return ret;
> > +
> > + dev_info(ph->dev, "NXP SM MISC Version %d.%d\n",
> > + PROTOCOL_REV_MAJOR(version),
> PROTOCOL_REV_MINOR(version));
> > +
> > + minfo = devm_kzalloc(ph->dev, sizeof(*minfo), GFP_KERNEL);
> > + if (!minfo)
> > + return -ENOMEM;
> > +
> > + ret = scmi_imx_misc_attributes_get(ph, minfo);
> > + if (ret)
> > + return ret;
> > +
> > + return ph->set_priv(ph, minfo, version); }
> > +
> > +static int scmi_imx_misc_ctrl_get(const struct scmi_protocol_handle *ph,
> > + u32 ctrl_id, u32 *num, u32 *val) {
> > + struct scmi_imx_misc_ctrl_get_out *out;
> > + struct scmi_xfer *t;
> > + int ret, i;
> > +
> > + ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_GET,
> sizeof(u32),
> > + 0, &t);
> > + if (ret)
> > + return ret;
> > +
> > + put_unaligned_le32(ctrl_id, t->tx.buf);
> > + ret = ph->xops->do_xfer(ph, t);
> > + if (!ret) {
> > + out = t->rx.buf;
> > + *num = le32_to_cpu(out->num);
> > + for (i = 0; i < *num && i < MISC_MAX_VAL; i++)
> > + val[i] = le32_to_cpu(out->val[i]);
> > + }
> > +
> > + ph->xops->xfer_put(ph, t);
> > +
> > + return ret;
> > +}
> > +
> > +static int scmi_imx_misc_ctrl_set(const struct scmi_protocol_handle *ph,
> > + u32 ctrl_id, u32 num, u32 *val) {
> > + struct scmi_imx_misc_ctrl_set_in *in;
> > + struct scmi_xfer *t;
> > + int ret, i;
> > +
> > + ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id);
> > + if (ret)
> > + return ret;
> > +
> > + if (num > MISC_MAX_VAL)
> > + return -EINVAL;
> > +
> > + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_SET,
> sizeof(*in),
> > + 0, &t);
> > + if (ret)
> > + return ret;
> > +
> > + in = t->tx.buf;
> > + in->id = cpu_to_le32(ctrl_id);
> > + in->num = cpu_to_le32(num);
> > + for (i = 0; i < num; i++)
> > + in->value[i] = cpu_to_le32(val[i]);
> > +
> > + ret = ph->xops->do_xfer(ph, t);
> > +
> > + ph->xops->xfer_put(ph, t);
> > +
> > + return ret;
> > +}
> > +
> > +static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops =
> {
> > + .misc_ctrl_set = scmi_imx_misc_ctrl_set,
> > + .misc_ctrl_get = scmi_imx_misc_ctrl_get, };
> > +
> > +static const struct scmi_protocol scmi_imx_misc = {
> > + .id = SCMI_PROTOCOL_IMX_MISC,
> > + .owner = THIS_MODULE,
> > + .instance_init = &scmi_imx_misc_protocol_init,
> > + .ops = &scmi_imx_misc_proto_ops,
> > + .events = &scmi_imx_misc_protocol_events,
> > + .supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION, };
> > +module_scmi_protocol(scmi_imx_misc);
> > diff --git a/include/linux/scmi_imx_protocol.h
> > b/include/linux/scmi_imx_protocol.h
> > index 90ce011a4429..a69bd4a20f0f 100644
> > --- a/include/linux/scmi_imx_protocol.h
> > +++ b/include/linux/scmi_imx_protocol.h
> > @@ -13,8 +13,14 @@
> > #include <linux/notifier.h>
> > #include <linux/types.h>
> >
> > +#define SCMI_PAYLOAD_LEN 100
> > +
> > +#define SCMI_ARRAY(X, Y) ((SCMI_PAYLOAD_LEN - (X)) / sizeof(Y))
> > +#define MISC_MAX_VAL SCMI_ARRAY(8, uint32_t)
> > +
> > enum scmi_nxp_protocol {
> > SCMI_PROTOCOL_IMX_BBM = 0x81,
> > + SCMI_PROTOCOL_IMX_MISC = 0x84,
> > };
> >
> > struct scmi_imx_bbm_proto_ops {
> > @@ -42,4 +48,15 @@ struct scmi_imx_bbm_notif_report {
> > unsigned int rtc_id;
> > unsigned int rtc_evt;
> > };
> > +
> > +struct scmi_imx_misc_ctrl_notify_report {
> > + ktime_t timestamp;
> > + unsigned int ctrl_id;
> > + unsigned int flags;
> > +};
> > +
> > +struct scmi_imx_misc_proto_ops {
> > + int (*misc_ctrl_set)(const struct scmi_protocol_handle *ph, u32 id,
> u32 num, u32 *val);
> > + int (*misc_ctrl_get)(const struct scmi_protocol_handle *ph, u32 id,
> > +u32 *num, u32 *val); };
> > #endif
> >
> > --
> > 2.37.1
> >
> >
> >
^ permalink raw reply
* RE: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI Extension protocol
From: Peng Fan @ 2024-04-07 1:50 UTC (permalink / raw)
To: Krzysztof Kozlowski, Peng Fan (OSS), Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Sudeep Holla,
Cristian Marussi
Cc: devicetree@vger.kernel.org, imx@lists.linux.dev,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <DU0PR04MB941719A18C3F749E7D180FD888012@DU0PR04MB9417.eurprd04.prod.outlook.com>
> Subject: RE: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI Extension
> protocol
>
> > Subject: Re: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI
> > Extension protocol
> >
> > On 05/04/2024 14:39, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > Add i.MX SCMI Extension protocols bindings for:
> > > - Battery Backed Secure Module(BBSM)
> >
> > Which is what?
>
> I should say BBM(BBSM + BBNSM), BBM has RTC and ON/OFF key features,
> but BBM is managed by SCMI firmware and exported to agent by BBM
> protocol. So add bindings for i.MX BBM protocol.
>
> Is this ok?
>
> >
> > > - MISC settings such as General Purpose Registers settings.
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > > .../devicetree/bindings/firmware/imx,scmi.yaml | 80
> > ++++++++++++++++++++++
> > > 1 file changed, 80 insertions(+)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> > > b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> > > new file mode 100644
> > > index 000000000000..7ee19a661d83
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> > > @@ -0,0 +1,80 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) # Copyright
> > > +2024 NXP %YAML 1.2
> > > +---
> > > +$id:
> > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fde
> > > +vi
> > >
> >
> +cetree.org%2Fschemas%2Ffirmware%2Fimx%2Cscmi.yaml%23&data=05%7
> > C02%7Cp
> > >
> >
> +eng.fan%40nxp.com%7C5d16781d3eca425a342508dc562910b7%7C686ea
> > 1d3bc2b4c
> > >
> > +6fa92cd99c5c301635%7C0%7C0%7C638479981570959816%7CUnknown%
> > 7CTWFpbGZsb
> > >
> >
> +3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
> > 0%3D
> > >
> >
> +%7C0%7C%7C%7C&sdata=mWNwPvu2eyF18MroVOBHb%2Fjeo%2BIHfV5V
> > h%2F9ebdx65MM
> > > +%3D&reserved=0
> > > +$schema:
> > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fde
> > > +vi
> > > +cetree.org%2Fmeta-
> > schemas%2Fcore.yaml%23&data=05%7C02%7Cpeng.fan%40nx
> > >
> >
> +p.com%7C5d16781d3eca425a342508dc562910b7%7C686ea1d3bc2b4c6fa
> > 92cd99c5c
> > >
> >
> +301635%7C0%7C0%7C638479981570971949%7CUnknown%7CTWFpbGZs
> > b3d8eyJWIjoiM
> > >
> >
> +C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7
> > C%7C%7
> > >
> >
> +C&sdata=v4XnGG00D4I8j5MJvDUVYMRTm7yRrvz0V3fUyc5KAAA%3D&reser
> > ved=0
> > > +
> > > +title: i.MX System Control and Management Interface(SCMI) Vendor
> > > +Protocols Extension
> > > +
> > > +maintainers:
> > > + - Peng Fan <peng.fan@nxp.com>
> > > +
> > > +allOf:
> > > + - $ref: arm,scmi.yaml#
> >
> > Sorry, but arm,scmi is a final schema. Is your plan to define some
> > common part?
>
> No. I just wanna add vendor extension per SCMI spec.
>
> 0x80-0xFF:
> Reserved for vendor or platform-specific extensions to this interface
>
> Each vendor may have different usage saying id 0x81, so I add i.MX dt-
> schema file.
>
> >
> > > +
> > > +properties:
> > > + protocol@81:
> > > + $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> > > + unevaluatedProperties: false
> > > + description:
> > > + The BBM Protocol is for managing Battery Backed Secure Module
> > (BBSM) RTC
> > > + and the ON/OFF Key
> > > +
> > > + properties:
> > > + reg:
> > > + const: 0x81
> > > +
> > > + required:
> > > + - reg
> > > +
> > > + protocol@84:
> > > + $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> > > + unevaluatedProperties: false
> > > + description:
> > > + The MISC Protocol is for managing SoC Misc settings, such as
> > > + GPR settings
> >
> > Genera register is not a setting... this is a pleonasm. Please be more
> > specific what is the GPR, MISC protocol etc.
>
> The MISC Protocol is for managing SoC Misc settings, such as SAI MCLK/MQS
> in Always On domain BLK CTRL, SAI_CLK_SEL in WAKEUP BLK CTRL, gpio
> expanders which is under control of SCMI firmware.
>
> > > +
> > > + properties:
> > > + reg:
> > > + const: 0x84
> > > +
> > > + wakeup-sources:
> > > + description:
> > > + Each entry consists of 2 integers, represents the source
> > > + and electric signal edge
> >
> > Can you answer questions from reviewers?
>
> Sorry. Is this ok?
> minItems: 1
> maxItems: 32
>
> >
> > > + items:
> > > + items:
> > > + - description: the wakeup source
> > > + - description: the wakeup electric signal edge
> > > + $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > > +
> > > + required:
> > > + - reg
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > + - |
> > > + firmware {
> > > + scmi {
> > > + compatible = "arm,scmi";
> >
> > > + mboxes = <&mu2 5 0>, <&mu2 3 0>, <&mu2 3 1>;
> > > + shmem = <&scmi_buf0>, <&scmi_buf1>;
> > > +
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + protocol@81 {
> > > + reg = <0x81>;
> > > + };
> > > +
> > > + protocol@84 {
> > > + reg = <0x84>;
> > > + wakeup-sources = <0x8000 1
> > > + 0x8001 1
> > > + 0x8002 1
> > > + 0x8003 1
> > > + 0x8004 1>;
> >
> > Nothing improved... If you are going to ignore reviews, then you will
> > only get NAKed.
>
> Sorry, you mean the examples, or the whole dt-schema?
Missed Rob's comment, will use
< > for each entry.
Thanks,
Peng.
>
> Thanks,
> Peng.
> >
> > Best regards,
> > Krzysztof
^ permalink raw reply
* RE: [PATCH v8 4/4] pinctrl: Implementation of the generic scmi-pinctrl driver
From: Peng Fan @ 2024-04-07 3:17 UTC (permalink / raw)
To: Peng Fan (OSS), Sudeep Holla, Cristian Marussi, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Linus Walleij, Dan Carpenter
Cc: Andy Shevchenko, 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: <20240405-pinctrl-scmi-v8-4-5fc8e33871bf@nxp.com>
> Subject: [PATCH v8 4/4] pinctrl: Implementation of the generic scmi-pinctrl
> driver
>
> 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.
Considering pinctrl expects ENOTSUPP, so I would add this in v9.
diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
index 0f55f000a679..accebe3f0706 100644
--- a/drivers/pinctrl/pinctrl-scmi.c
+++ b/drivers/pinctrl/pinctrl-scmi.c
@@ -296,8 +296,12 @@ static int pinctrl_scmi_pinconf_get(struct pinctrl_dev *pctldev,
ret = pinctrl_ops->settings_get_one(pmx->ph, pin, PIN_TYPE, type,
&config_value);
- if (ret)
+ if (ret) {
+ /* Convert SCMI error code to PINCTRL expected error code */
+ if (ret == -EOPNOTSUPP)
+ ret = -ENOTSUPP;
return ret;
+ }
*config = pinconf_to_config_packed(config_type, config_value);
@@ -445,8 +449,12 @@ static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev,
ret = pinctrl_ops->settings_get_one(pmx->ph, group, GROUP_TYPE, type,
&config_value);
- if (ret)
+ if (ret) {
+ /* Convert SCMI error code to PINCTRL expected error code */
+ if (ret == -EOPNOTSUPP)
+ ret = -ENOTSUPP;
return ret;
+ }
*config = pinconf_to_config_packed(config_type, config_value);
I will collect more comments before post v9. Just post out upper diff
to see any disagreements.
Thanks,
Peng.
>
> Co-developed-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> MAINTAINERS | 1 +
> drivers/pinctrl/Kconfig | 11 +
> drivers/pinctrl/Makefile | 1 +
> drivers/pinctrl/pinctrl-scmi.c | 564
> +++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 577 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4b511a55101c..d8270ac6651a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21457,6 +21457,7 @@ F: drivers/cpufreq/sc[mp]i-cpufreq.c
> F: drivers/firmware/arm_scmi/
> F: drivers/firmware/arm_scpi.c
> F: drivers/hwmon/scmi-hwmon.c
> +F: drivers/pinctrl/pinctrl-scmi.c
> F: drivers/pmdomain/arm/
> F: drivers/powercap/arm_scmi_powercap.c
> F: drivers/regulator/scmi-regulator.c
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index
> d45657aa986a..4e6f65cf0e76 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -450,6 +450,17 @@ config PINCTRL_ROCKCHIP
> help
> This support pinctrl and GPIO driver for Rockchip SoCs.
>
> +config PINCTRL_SCMI
> + tristate "Pinctrl driver using SCMI protocol interface"
> + depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
> + select PINMUX
> + select GENERIC_PINCONF
> + help
> + This driver provides support for pinctrl which is controlled
> + by firmware that implements the SCMI interface.
> + It uses SCMI Message Protocol to interact with the
> + firmware providing all the pinctrl controls.
> +
> config PINCTRL_SINGLE
> tristate "One-register-per-pin type device tree based pinctrl driver"
> depends on OF
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index
> 2152539b53d5..cc809669405a 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -45,6 +45,7 @@ obj-$(CONFIG_PINCTRL_PIC32) += pinctrl-pic32.o
> obj-$(CONFIG_PINCTRL_PISTACHIO) += pinctrl-pistachio.o
> obj-$(CONFIG_PINCTRL_RK805) += pinctrl-rk805.o
> obj-$(CONFIG_PINCTRL_ROCKCHIP) += pinctrl-rockchip.o
> +obj-$(CONFIG_PINCTRL_SCMI) += pinctrl-scmi.o
> obj-$(CONFIG_PINCTRL_SINGLE) += pinctrl-single.o
> obj-$(CONFIG_PINCTRL_ST) += pinctrl-st.o
> obj-$(CONFIG_PINCTRL_STMFX) += pinctrl-stmfx.o
> diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c new
> file mode 100644 index 000000000000..0f55f000a679
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-scmi.c
> @@ -0,0 +1,564 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Power Interface (SCMI) Protocol based pinctrl
> +driver
> + *
> + * Copyright (C) 2024 EPAM
> + * Copyright 2024 NXP
> + */
> +
> +#include <linux/device.h>
> +#include <linux/dev_printk.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +#include <linux/pinctrl/machine.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinconf-generic.h> #include
> +<linux/pinctrl/pinctrl.h> #include <linux/pinctrl/pinmux.h>
> +
> +#include "pinctrl-utils.h"
> +#include "core.h"
> +#include "pinconf.h"
> +
> +#define DRV_NAME "scmi-pinctrl"
> +
> +/* Define num configs, if not large than 4 use stack, else use kcalloc */
> +#define SCMI_NUM_CONFIGS 4
> +
> +static const struct scmi_pinctrl_proto_ops *pinctrl_ops;
> +
> +struct scmi_pinctrl {
> + struct device *dev;
> + struct scmi_protocol_handle *ph;
> + struct pinctrl_dev *pctldev;
> + struct pinctrl_desc pctl_desc;
> + struct pinfunction *functions;
> + unsigned int nr_functions;
> + struct pinctrl_pin_desc *pins;
> + unsigned int nr_pins;
> +};
> +
> +static int pinctrl_scmi_get_groups_count(struct pinctrl_dev *pctldev) {
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + return pinctrl_ops->count_get(pmx->ph, GROUP_TYPE); }
> +
> +static const char *pinctrl_scmi_get_group_name(struct pinctrl_dev *pctldev,
> + unsigned int selector)
> +{
> + int ret;
> + const char *name;
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + ret = pinctrl_ops->name_get(pmx->ph, selector, GROUP_TYPE,
> &name);
> + if (ret) {
> + dev_err(pmx->dev, "get name failed with err %d", ret);
> + return NULL;
> + }
> +
> + return name;
> +}
> +
> +static int pinctrl_scmi_get_group_pins(struct pinctrl_dev *pctldev,
> + unsigned int selector,
> + const unsigned int **pins,
> + unsigned int *num_pins)
> +{
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + return pinctrl_ops->group_pins_get(pmx->ph, selector, pins,
> num_pins);
> +}
> +
> +static const struct pinctrl_ops pinctrl_scmi_pinctrl_ops = {
> + .get_groups_count = pinctrl_scmi_get_groups_count,
> + .get_group_name = pinctrl_scmi_get_group_name,
> + .get_group_pins = pinctrl_scmi_get_group_pins, #ifdef CONFIG_OF
> + .dt_node_to_map = pinconf_generic_dt_node_to_map_all,
> + .dt_free_map = pinconf_generic_dt_free_map, #endif };
> +
> +static int pinctrl_scmi_get_functions_count(struct pinctrl_dev
> +*pctldev) {
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + return pinctrl_ops->count_get(pmx->ph, FUNCTION_TYPE); }
> +
> +static const char *pinctrl_scmi_get_function_name(struct pinctrl_dev
> *pctldev,
> + unsigned int selector)
> +{
> + int ret;
> + const char *name;
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + ret = pinctrl_ops->name_get(pmx->ph, selector, FUNCTION_TYPE,
> &name);
> + if (ret) {
> + dev_err(pmx->dev, "get name failed with err %d", ret);
> + return NULL;
> + }
> +
> + return name;
> +}
> +
> +static int pinctrl_scmi_get_function_groups(struct pinctrl_dev *pctldev,
> + unsigned int selector,
> + const char * const **p_groups,
> + unsigned int * const
> p_num_groups) {
> + struct pinfunction *func;
> + const unsigned int *group_ids;
> + unsigned int num_groups;
> + const char **groups;
> + int ret, i;
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!p_groups || !p_num_groups)
> + return -EINVAL;
> +
> + if (selector >= pmx->nr_functions)
> + return -EINVAL;
> +
> + func = &pmx->functions[selector];
> + if (func->ngroups)
> + goto done;
> +
> + ret = pinctrl_ops->function_groups_get(pmx->ph, selector,
> &num_groups,
> + &group_ids);
> + if (ret) {
> + dev_err(pmx->dev, "Unable to get function groups, err %d",
> ret);
> + return ret;
> + }
> + if (!num_groups)
> + return -EINVAL;
> +
> + groups = kcalloc(num_groups, sizeof(*groups), GFP_KERNEL);
> + if (!groups)
> + return -ENOMEM;
> +
> + for (i = 0; i < num_groups; i++) {
> + groups[i] = pinctrl_scmi_get_group_name(pctldev,
> group_ids[i]);
> + if (!groups[i]) {
> + ret = -EINVAL;
> + goto err_free;
> + }
> + }
> +
> + func->ngroups = num_groups;
> + func->groups = groups;
> +done:
> + *p_groups = func->groups;
> + *p_num_groups = func->ngroups;
> +
> + return 0;
> +
> +err_free:
> + kfree(groups);
> +
> + return ret;
> +}
> +
> +static int pinctrl_scmi_func_set_mux(struct pinctrl_dev *pctldev,
> + unsigned int selector, unsigned int group)
> {
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + return pinctrl_ops->mux_set(pmx->ph, selector, group); }
> +
> +static int pinctrl_scmi_request(struct pinctrl_dev *pctldev,
> + unsigned int offset)
> +{
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + return pinctrl_ops->pin_request(pmx->ph, offset); }
> +
> +static int pinctrl_scmi_free(struct pinctrl_dev *pctldev, unsigned int
> +offset) {
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + return pinctrl_ops->pin_free(pmx->ph, offset); }
> +
> +static const struct pinmux_ops pinctrl_scmi_pinmux_ops = {
> + .request = pinctrl_scmi_request,
> + .free = pinctrl_scmi_free,
> + .get_functions_count = pinctrl_scmi_get_functions_count,
> + .get_function_name = pinctrl_scmi_get_function_name,
> + .get_function_groups = pinctrl_scmi_get_function_groups,
> + .set_mux = pinctrl_scmi_func_set_mux,
> +};
> +
> +static int pinctrl_scmi_map_pinconf_type(enum pin_config_param param,
> + enum scmi_pinctrl_conf_type
> *type) {
> + u32 arg = param;
> +
> + switch (arg) {
> + case PIN_CONFIG_BIAS_BUS_HOLD:
> + *type = SCMI_PIN_BIAS_BUS_HOLD;
> + break;
> + case PIN_CONFIG_BIAS_DISABLE:
> + *type = SCMI_PIN_BIAS_DISABLE;
> + break;
> + case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
> + *type = SCMI_PIN_BIAS_HIGH_IMPEDANCE;
> + break;
> + case PIN_CONFIG_BIAS_PULL_DOWN:
> + *type = SCMI_PIN_BIAS_PULL_DOWN;
> + break;
> + case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
> + *type = SCMI_PIN_BIAS_PULL_DEFAULT;
> + break;
> + case PIN_CONFIG_BIAS_PULL_UP:
> + *type = SCMI_PIN_BIAS_PULL_UP;
> + break;
> + case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> + *type = SCMI_PIN_DRIVE_OPEN_DRAIN;
> + break;
> + case PIN_CONFIG_DRIVE_OPEN_SOURCE:
> + *type = SCMI_PIN_DRIVE_OPEN_SOURCE;
> + break;
> + case PIN_CONFIG_DRIVE_PUSH_PULL:
> + *type = SCMI_PIN_DRIVE_PUSH_PULL;
> + break;
> + case PIN_CONFIG_DRIVE_STRENGTH:
> + *type = SCMI_PIN_DRIVE_STRENGTH;
> + break;
> + case PIN_CONFIG_DRIVE_STRENGTH_UA:
> + *type = SCMI_PIN_DRIVE_STRENGTH;
> + break;
> + case PIN_CONFIG_INPUT_DEBOUNCE:
> + *type = SCMI_PIN_INPUT_DEBOUNCE;
> + break;
> + case PIN_CONFIG_INPUT_ENABLE:
> + *type = SCMI_PIN_INPUT_MODE;
> + break;
> + case PIN_CONFIG_INPUT_SCHMITT:
> + *type = SCMI_PIN_INPUT_SCHMITT;
> + break;
> + case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> + *type = SCMI_PIN_INPUT_MODE;
> + break;
> + case PIN_CONFIG_MODE_LOW_POWER:
> + *type = SCMI_PIN_LOW_POWER_MODE;
> + break;
> + case PIN_CONFIG_OUTPUT:
> + *type = SCMI_PIN_OUTPUT_VALUE;
> + break;
> + case PIN_CONFIG_OUTPUT_ENABLE:
> + *type = SCMI_PIN_OUTPUT_MODE;
> + break;
> + case PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS:
> + *type = SCMI_PIN_OUTPUT_VALUE;
> + break;
> + case PIN_CONFIG_POWER_SOURCE:
> + *type = SCMI_PIN_POWER_SOURCE;
> + break;
> + case PIN_CONFIG_SLEW_RATE:
> + *type = SCMI_PIN_SLEW_RATE;
> + break;
> + case SCMI_PIN_OEM_START ... SCMI_PIN_OEM_END:
> + *type = arg;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int pinctrl_scmi_pinconf_get(struct pinctrl_dev *pctldev,
> + unsigned int pin, unsigned long *config) {
> + int ret;
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> + enum pin_config_param config_type;
> + enum scmi_pinctrl_conf_type type;
> + u32 config_value;
> +
> + if (!config)
> + return -EINVAL;
> +
> + config_type = pinconf_to_config_param(*config);
> +
> + ret = pinctrl_scmi_map_pinconf_type(config_type, &type);
> + if (ret)
> + return ret;
> +
> + ret = pinctrl_ops->settings_get_one(pmx->ph, pin, PIN_TYPE, type,
> + &config_value);
> + if (ret)
> + return ret;
> +
> + *config = pinconf_to_config_packed(config_type, config_value);
> +
> + return 0;
> +}
> +
> +static int
> +pinctrl_scmi_alloc_configs(struct pinctrl_dev *pctldev, u32 num_configs,
> + u32 **p_config_value,
> + enum scmi_pinctrl_conf_type **p_config_type) {
> + if (num_configs <= SCMI_NUM_CONFIGS)
> + return 0;
> +
> + *p_config_value = kcalloc(num_configs, sizeof(**p_config_value),
> GFP_KERNEL);
> + if (!*p_config_value)
> + return -ENOMEM;
> +
> + *p_config_type = kcalloc(num_configs, sizeof(**p_config_type),
> GFP_KERNEL);
> + if (!*p_config_type) {
> + kfree(*p_config_value);
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +static void
> +pinctrl_scmi_free_configs(struct pinctrl_dev *pctldev, u32 num_configs,
> + u32 **p_config_value,
> + enum scmi_pinctrl_conf_type **p_config_type) {
> + if (num_configs <= SCMI_NUM_CONFIGS)
> + return;
> +
> + kfree(*p_config_value);
> + kfree(*p_config_type);
> +}
> +
> +static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev,
> + unsigned int pin,
> + unsigned long *configs,
> + unsigned int num_configs)
> +{
> + int i, ret;
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> + enum scmi_pinctrl_conf_type config_type[SCMI_NUM_CONFIGS];
> + u32 config_value[SCMI_NUM_CONFIGS];
> + enum scmi_pinctrl_conf_type *p_config_type = config_type;
> + u32 *p_config_value = config_value;
> + enum pin_config_param param;
> +
> + if (!configs || !num_configs)
> + return -EINVAL;
> +
> + ret = pinctrl_scmi_alloc_configs(pctldev, num_configs,
> &p_config_type,
> + &p_config_value);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < num_configs; i++) {
> + param = pinconf_to_config_param(configs[i]);
> + ret = pinctrl_scmi_map_pinconf_type(param,
> &p_config_type[i]);
> + if (ret) {
> + dev_err(pmx->dev, "Error map pinconf_type %d\n",
> ret);
> + goto free_config;
> + }
> + p_config_value[i] = pinconf_to_config_argument(configs[i]);
> + }
> +
> + ret = pinctrl_ops->settings_conf(pmx->ph, pin, PIN_TYPE,
> num_configs,
> + p_config_type, p_config_value);
> + if (ret)
> + dev_err(pmx->dev, "Error parsing config %d\n", ret);
> +
> +free_config:
> + pinctrl_scmi_free_configs(pctldev, num_configs, &p_config_type,
> + &p_config_value);
> + return ret;
> +}
> +
> +static int pinctrl_scmi_pinconf_group_set(struct pinctrl_dev *pctldev,
> + unsigned int group,
> + unsigned long *configs,
> + unsigned int num_configs)
> +{
> + int i, ret;
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> + enum scmi_pinctrl_conf_type config_type[SCMI_NUM_CONFIGS];
> + u32 config_value[SCMI_NUM_CONFIGS];
> + enum scmi_pinctrl_conf_type *p_config_type = config_type;
> + u32 *p_config_value = config_value;
> + enum pin_config_param param;
> +
> + if (!configs || !num_configs)
> + return -EINVAL;
> +
> + ret = pinctrl_scmi_alloc_configs(pctldev, num_configs,
> &p_config_type,
> + &p_config_value);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < num_configs; i++) {
> + param = pinconf_to_config_param(configs[i]);
> + ret = pinctrl_scmi_map_pinconf_type(param,
> &p_config_type[i]);
> + if (ret) {
> + dev_err(pmx->dev, "Error map pinconf_type %d\n",
> ret);
> + goto free_config;
> + }
> +
> + p_config_value[i] = pinconf_to_config_argument(configs[i]);
> + }
> +
> + ret = pinctrl_ops->settings_conf(pmx->ph, group, GROUP_TYPE,
> + num_configs, p_config_type,
> + p_config_value);
> + if (ret)
> + dev_err(pmx->dev, "Error parsing config %d", ret);
> +
> +free_config:
> + pinctrl_scmi_free_configs(pctldev, num_configs, &p_config_type,
> + &p_config_value);
> + return ret;
> +};
> +
> +static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev,
> + unsigned int group,
> + unsigned long *config)
> +{
> + int ret;
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> + enum pin_config_param config_type;
> + enum scmi_pinctrl_conf_type type;
> + u32 config_value;
> +
> + if (!config)
> + return -EINVAL;
> +
> + config_type = pinconf_to_config_param(*config);
> + ret = pinctrl_scmi_map_pinconf_type(config_type, &type);
> + if (ret) {
> + dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
> + return ret;
> + }
> +
> + ret = pinctrl_ops->settings_get_one(pmx->ph, group, GROUP_TYPE,
> type,
> + &config_value);
> + if (ret)
> + return ret;
> +
> + *config = pinconf_to_config_packed(config_type, config_value);
> +
> + return 0;
> +}
> +
> +static const struct pinconf_ops pinctrl_scmi_pinconf_ops = {
> + .is_generic = true,
> + .pin_config_get = pinctrl_scmi_pinconf_get,
> + .pin_config_set = pinctrl_scmi_pinconf_set,
> + .pin_config_group_set = pinctrl_scmi_pinconf_group_set,
> + .pin_config_group_get = pinctrl_scmi_pinconf_group_get,
> + .pin_config_config_dbg_show = pinconf_generic_dump_config, };
> +
> +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.
> + */
> + pins = devm_kmalloc_array(pmx->dev, npins, sizeof(*pins),
> GFP_KERNEL);
> + if (!pins)
> + return -ENOMEM;
> +
> + for (i = 0; i < npins; i++) {
> + pins[i].number = i;
> + /*
> + * The memory for name is handled by the scmi firmware
> driver,
> + * no need free here
> + */
> + ret = pinctrl_ops->name_get(pmx->ph, i, PIN_TYPE,
> &pins[i].name);
> + if (ret)
> + return dev_err_probe(pmx->dev, ret,
> + "Can't get name for pin %d", i);
> + }
> +
> + desc->npins = npins;
> + desc->pins = pins;
> + dev_dbg(pmx->dev, "got pins %u", npins);
> +
> + return 0;
> +}
> +
> +static int scmi_pinctrl_probe(struct scmi_device *sdev) {
> + int ret;
> + struct device *dev = &sdev->dev;
> + struct scmi_pinctrl *pmx;
> + const struct scmi_handle *handle;
> + struct scmi_protocol_handle *ph;
> +
> + if (!sdev->handle)
> + return -EINVAL;
> +
> + handle = sdev->handle;
> +
> + pinctrl_ops = handle->devm_protocol_get(sdev,
> SCMI_PROTOCOL_PINCTRL, &ph);
> + if (IS_ERR(pinctrl_ops))
> + return PTR_ERR(pinctrl_ops);
> +
> + pmx = devm_kzalloc(dev, sizeof(*pmx), GFP_KERNEL);
> + if (!pmx)
> + return -ENOMEM;
> +
> + pmx->ph = ph;
> +
> + pmx->dev = dev;
> + pmx->pctl_desc.name = DRV_NAME;
> + pmx->pctl_desc.owner = THIS_MODULE;
> + pmx->pctl_desc.pctlops = &pinctrl_scmi_pinctrl_ops;
> + pmx->pctl_desc.pmxops = &pinctrl_scmi_pinmux_ops;
> + pmx->pctl_desc.confops = &pinctrl_scmi_pinconf_ops;
> +
> + ret = pinctrl_scmi_get_pins(pmx, &pmx->pctl_desc);
> + if (ret)
> + return ret;
> +
> + ret = devm_pinctrl_register_and_init(dev, &pmx->pctl_desc, pmx,
> + &pmx->pctldev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to register pinctrl\n");
> +
> + pmx->nr_functions = pinctrl_scmi_get_functions_count(pmx-
> >pctldev);
> + pmx->functions = devm_kcalloc(dev, pmx->nr_functions,
> + sizeof(*pmx->functions), GFP_KERNEL);
> + if (!pmx->functions)
> + return -ENOMEM;
> +
> + return pinctrl_enable(pmx->pctldev);
> +}
> +
> +static const struct scmi_device_id scmi_id_table[] = {
> + { SCMI_PROTOCOL_PINCTRL, "pinctrl" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(scmi, scmi_id_table);
> +
> +static struct scmi_driver scmi_pinctrl_driver = {
> + .name = DRV_NAME,
> + .probe = scmi_pinctrl_probe,
> + .id_table = scmi_id_table,
> +};
> +module_scmi_driver(scmi_pinctrl_driver);
> +
> +MODULE_AUTHOR("Oleksii Moisieiev <oleksii_moisieiev@epam.com>");
> +MODULE_AUTHOR("Peng Fan <peng.fan@nxp.com>");
> MODULE_DESCRIPTION("ARM
> +SCMI pin controller driver"); MODULE_LICENSE("GPL");
>
> --
> 2.37.1
^ permalink raw reply related
* [PATCH v9 0/6] PCI: qcom: Add support for OPP
From: Krishna chaitanya chundru @ 2024-04-07 4:37 UTC (permalink / raw)
To: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Bjorn Helgaas, johan+linaro, bmasney,
djakov
Cc: linux-arm-msm, devicetree, linux-kernel, linux-pci, vireshk,
quic_vbadigan, quic_skananth, quic_nitegupt, quic_parass,
quic_krichai, krzysztof.kozlowski, Bryan O'Donoghue
This patch adds support for OPP to vote for the performance state of RPMH
power domain based upon PCIe speed it got enumerated.
QCOM Resource Power Manager-hardened (RPMh) is a hardware block which
maintains hardware state of a regulator by performing max aggregation of
the requests made by all of the processors.
PCIe controller can operate on different RPMh performance state of power
domain based up on the speed of the link. And this performance state varies
from target to target.
It is manadate to scale the performance state based up on the PCIe speed
link operates so that SoC can run under optimum power conditions.
Add Operating Performance Points(OPP) support to vote for RPMh state based
upon GEN speed link is operating.
Before link up PCIe driver will vote for the maximum performance state.
As now we are adding ICC BW vote in OPP, the ICC BW voting depends both
GEN speed and link width using opp-level to indicate the opp entry table
will be difficult.
In PCIe certain gen speeds like GEN1x2 & GEN2X1 or GEN3x2 & GEN4x1 use
same icc bw if we use freq in the OPP table to represent the PCIe Gen
speed number of PCIe entries can reduced.
So going back to use freq in the OPP table instead of level.
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
Changes from v8:
- Removed the ack-by and reviewed by on dt-bindings as dt-bindings moved to new files.
- Removed dt-binding patch for interconnects as it is added in the common file.
- Added tags for interconnect as suggested by konrad
- Added the comments as suggested by mani
- In ICC BW vote for CPU to PCIe path if icc_disable() fails log error and return instead of re-init.
- Link to v8: https://lore.kernel.org/linux-arm-msm/20240302-opp_support-v8-0-158285b86b10@quicinc.com/
Changes from v7:
- Fix the compilation issue in patch3
- Change the commit text and wrap the comments to 80 columns as suggested by bjorn
- remove PCIE_MBS2FREQ macro as this is being used by only qcom drivers.
- Link to v7: https://lore.kernel.org/r/20240223-opp_support-v7-0-10b4363d7e71@quicinc.com
Changes from v6:
- change CPU-PCIe bandwidth to 1KBps as suggested by HW team.
- Create a new API to get frequency based upon PCIe speed as suggested
by mani.
- Updated few commit texts and comments.
- Setting opp to NULL in suspend to remove any votes.
- Link for v6: https://lore.kernel.org/linux-arm-msm/20240112-opp_support-v6-0-77bbf7d0cc37@quicinc.com/
Changes from v5:
- Add ICC BW voting as part of OPP, rebase the latest kernel, and only
- either OPP or ICC BW voting will supported we removed the patch to
- return error for icc opp update patch.
- As we added the icc bw voting in opp table I am not including reviewed
- by tags given in previous patch.
- Use opp freq to find opp entries as now we need to include pcie link
- also in to considerations.
- Add CPU-PCIe BW voting which is not present till now.
- Drop PCI: qcom: Return error from 'qcom_pcie_icc_update' as either opp or icc bw
- only one executes and there is no need to fail if opp or icc update fails.
- Link for v5: https://lore.kernel.org/linux-arm-msm/20231101063323.GH2897@thinkpad/T/
Changes from v4:
- Added a separate patch for returning error from the qcom_pcie_upadate
and moved opp update logic to icc_update and used a bool variable to
update the opp.
- Addressed comments made by pavan.
changes from v3:
- Removing the opp vote on suspend when the link is not up and link is not
up and add debug prints as suggested by pavan.
- Added dev_pm_opp_find_level_floor API to find the highest opp to vote.
changes from v2:
- Instead of using the freq based opp search use level based as suggested
by Dmitry Baryshkov.
Changes from v1:
- Addressed comments from Krzysztof Kozlowski.
- Added the rpmhpd_opp_xxx phandle as suggested by pavan.
- Added dev_pm_opp_set_opp API call which was missed on previous patch.
---
---
Krishna chaitanya chundru (6):
arm64: dts: qcom: sm8450: Add interconnect path to PCIe node
PCI: qcom: Add ICC bandwidth vote for CPU to PCIe path
dt-bindings: pci: qcom: Add opp table
arm64: dts: qcom: sm8450: Add opp table support to PCIe
PCI: Bring the PCIe speed to MBps logic to new pcie_link_speed_to_mbps()
PCI: qcom: Add OPP support to scale performance state of power domain
.../devicetree/bindings/pci/qcom,pcie-sm8450.yaml | 4 +
arch/arm64/boot/dts/qcom/sm8450.dtsi | 89 +++++++++++++++++
drivers/pci/controller/dwc/pcie-qcom.c | 108 +++++++++++++++++----
drivers/pci/pci.c | 19 +---
drivers/pci/pci.h | 22 +++++
5 files changed, 207 insertions(+), 35 deletions(-)
---
base-commit: 6c6e47d69d821047097909288b6d7f1aafb3b9b1
change-id: 20240406-opp_support-ca095eb032b4
Best regards,
--
Krishna chaitanya chundru <quic_krichai@quicinc.com>
^ permalink raw reply
* [PATCH v9 1/6] arm64: dts: qcom: sm8450: Add interconnect path to PCIe node
From: Krishna chaitanya chundru @ 2024-04-07 4:37 UTC (permalink / raw)
To: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Bjorn Helgaas, johan+linaro, bmasney,
djakov
Cc: linux-arm-msm, devicetree, linux-kernel, linux-pci, vireshk,
quic_vbadigan, quic_skananth, quic_nitegupt, quic_parass,
quic_krichai, krzysztof.kozlowski
In-Reply-To: <20240407-opp_support-v9-0-496184dc45d7@quicinc.com>
Add pcie-mem & cpu-pcie interconnect path to the PCIe nodes.
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
arch/arm64/boot/dts/qcom/sm8450.dtsi | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index b86be34a912b..615296e13c43 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -1807,6 +1807,12 @@ pcie0: pcie@1c00000 {
<0 0 0 3 &intc 0 0 0 151 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
<0 0 0 4 &intc 0 0 0 152 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
+ interconnects = <&pcie_noc MASTER_PCIE_0 QCOM_ICC_TAG_ALWAYS
+ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
+ <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
+ &config_noc SLAVE_PCIE_0 QCOM_ICC_TAG_ALWAYS>;
+ interconnect-names = "pcie-mem", "cpu-pcie";
+
clocks = <&gcc GCC_PCIE_0_PIPE_CLK>,
<&gcc GCC_PCIE_0_PIPE_CLK_SRC>,
<&pcie0_phy>,
@@ -1930,6 +1936,12 @@ pcie1: pcie@1c08000 {
<0 0 0 3 &intc 0 0 0 438 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
<0 0 0 4 &intc 0 0 0 439 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
+ interconnects = <&pcie_noc MASTER_PCIE_1 QCOM_ICC_TAG_ALWAYS
+ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
+ <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
+ &config_noc SLAVE_PCIE_1 QCOM_ICC_TAG_ALWAYS>;
+ interconnect-names = "pcie-mem", "cpu-pcie";
+
clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
<&gcc GCC_PCIE_1_PIPE_CLK_SRC>,
<&pcie1_phy>,
--
2.42.0
^ permalink raw reply related
* [PATCH v9 2/6] PCI: qcom: Add ICC bandwidth vote for CPU to PCIe path
From: Krishna chaitanya chundru @ 2024-04-07 4:37 UTC (permalink / raw)
To: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Bjorn Helgaas, johan+linaro, bmasney,
djakov
Cc: linux-arm-msm, devicetree, linux-kernel, linux-pci, vireshk,
quic_vbadigan, quic_skananth, quic_nitegupt, quic_parass,
quic_krichai, krzysztof.kozlowski, Bryan O'Donoghue
In-Reply-To: <20240407-opp_support-v9-0-496184dc45d7@quicinc.com>
To access PCIe registers, PCIe BAR space, config space the CPU-PCIe
ICC (interconnect consumers) path should be voted otherwise it may
lead to NoC (Network on chip) timeout. We are surviving because of
other driver vote for this path.
As there is less access on this path compared to PCIe to mem path
add minimum vote i.e 1KBps bandwidth always which is recommended
by HW team.
When suspending, disable this path after register space access
is done.
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
drivers/pci/controller/dwc/pcie-qcom.c | 38 ++++++++++++++++++++++++++++++----
1 file changed, 34 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 14772edcf0d3..b4893214b2d3 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -245,6 +245,7 @@ struct qcom_pcie {
struct phy *phy;
struct gpio_desc *reset;
struct icc_path *icc_mem;
+ struct icc_path *icc_cpu;
const struct qcom_pcie_cfg *cfg;
struct dentry *debugfs;
bool suspended;
@@ -1409,6 +1410,9 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
if (IS_ERR(pcie->icc_mem))
return PTR_ERR(pcie->icc_mem);
+ pcie->icc_cpu = devm_of_icc_get(pci->dev, "cpu-pcie");
+ if (IS_ERR(pcie->icc_cpu))
+ return PTR_ERR(pcie->icc_cpu);
/*
* Some Qualcomm platforms require interconnect bandwidth constraints
* to be set before enabling interconnect clocks.
@@ -1418,7 +1422,19 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
*/
ret = icc_set_bw(pcie->icc_mem, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
if (ret) {
- dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
+ dev_err(pci->dev, "failed to set interconnect bandwidth for PCIe-MEM: %d\n",
+ ret);
+ return ret;
+ }
+
+ /*
+ * Since the CPU-PCIe path is only used for activities like register
+ * access, Config/BAR space access, HW team has recommended to use a
+ * minimal bandwidth of 1KBps just to keep the link active.
+ */
+ ret = icc_set_bw(pcie->icc_cpu, 0, kBps_to_icc(1));
+ if (ret) {
+ dev_err(pci->dev, "failed to set interconnect bandwidth for CPU-PCIe: %d\n",
ret);
return ret;
}
@@ -1448,7 +1464,7 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
ret = icc_set_bw(pcie->icc_mem, 0, width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
if (ret) {
- dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
+ dev_err(pci->dev, "failed to set interconnect bandwidth for PCIe-MEM: %d\n",
ret);
}
}
@@ -1610,7 +1626,7 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
*/
ret = icc_set_bw(pcie->icc_mem, 0, kBps_to_icc(1));
if (ret) {
- dev_err(dev, "Failed to set interconnect bandwidth: %d\n", ret);
+ dev_err(dev, "Failed to set interconnect bandwidth for PCIe-MEM: %d\n", ret);
return ret;
}
@@ -1634,7 +1650,15 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
pcie->suspended = true;
}
- return 0;
+ /*
+ * Remove the vote for CPU-PCIe path now, since at this point onwards,
+ * no register access will be done.
+ */
+ ret = icc_disable(pcie->icc_cpu);
+ if (ret)
+ dev_err(dev, "failed to disable icc path of CPU-PCIe: %d\n", ret);
+
+ return ret;
}
static int qcom_pcie_resume_noirq(struct device *dev)
@@ -1642,6 +1666,12 @@ static int qcom_pcie_resume_noirq(struct device *dev)
struct qcom_pcie *pcie = dev_get_drvdata(dev);
int ret;
+ ret = icc_enable(pcie->icc_cpu);
+ if (ret) {
+ dev_err(dev, "failed to enable icc path of CPU-PCIe: %d\n", ret);
+ return ret;
+ }
+
if (pcie->suspended) {
ret = qcom_pcie_host_init(&pcie->pci->pp);
if (ret)
--
2.42.0
^ permalink raw reply related
* [PATCH v9 3/6] dt-bindings: pci: qcom: Add opp table
From: Krishna chaitanya chundru @ 2024-04-07 4:37 UTC (permalink / raw)
To: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Bjorn Helgaas, johan+linaro, bmasney,
djakov
Cc: linux-arm-msm, devicetree, linux-kernel, linux-pci, vireshk,
quic_vbadigan, quic_skananth, quic_nitegupt, quic_parass,
quic_krichai, krzysztof.kozlowski
In-Reply-To: <20240407-opp_support-v9-0-496184dc45d7@quicinc.com>
PCIe needs to choose the appropriate performance state of RPMH power
domain based upon the PCIe gen speed.
Adding the Operating Performance Points table allows to adjust power
domain performance state and icc peak bw, depending on the PCIe gen
speed and width.
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
Documentation/devicetree/bindings/pci/qcom,pcie-sm8450.yaml | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-sm8450.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-sm8450.yaml
index 1496d6993ab4..d8c0afaa4b19 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie-sm8450.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie-sm8450.yaml
@@ -69,6 +69,10 @@ properties:
- const: msi6
- const: msi7
+ operating-points-v2: true
+ opp-table:
+ type: object
+
resets:
maxItems: 1
--
2.42.0
^ permalink raw reply related
* [PATCH v9 4/6] arm64: dts: qcom: sm8450: Add opp table support to PCIe
From: Krishna chaitanya chundru @ 2024-04-07 4:37 UTC (permalink / raw)
To: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Bjorn Helgaas, johan+linaro, bmasney,
djakov
Cc: linux-arm-msm, devicetree, linux-kernel, linux-pci, vireshk,
quic_vbadigan, quic_skananth, quic_nitegupt, quic_parass,
quic_krichai, krzysztof.kozlowski
In-Reply-To: <20240407-opp_support-v9-0-496184dc45d7@quicinc.com>
PCIe needs to choose the appropriate performance state of RPMH power
domain and interconnect bandwidth based up on the PCIe gen speed.
Add the OPP table support to specify RPMH performance states and
interconnect peak bandwidth.
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
arch/arm64/boot/dts/qcom/sm8450.dtsi | 77 ++++++++++++++++++++++++++++++++++++
1 file changed, 77 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index 615296e13c43..881e5339cfff 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -1855,7 +1855,35 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
pinctrl-names = "default";
pinctrl-0 = <&pcie0_default_state>;
+ operating-points-v2 = <&pcie0_opp_table>;
+
status = "disabled";
+
+ pcie0_opp_table: opp-table {
+ compatible = "operating-points-v2";
+
+ /* GEN 1x1 */
+ opp-2500000 {
+ opp-hz = /bits/ 64 <2500000>;
+ required-opps = <&rpmhpd_opp_low_svs>;
+ opp-peak-kBps = <250000 1>;
+ };
+
+ /* GEN 2x1 */
+ opp-5000000 {
+ opp-hz = /bits/ 64 <5000000>;
+ required-opps = <&rpmhpd_opp_low_svs>;
+ opp-peak-kBps = <500000 1>;
+ };
+
+ /* GEN 3x1 */
+ opp-8000000 {
+ opp-hz = /bits/ 64 <8000000>;
+ required-opps = <&rpmhpd_opp_nom>;
+ opp-peak-kBps = <984500 1>;
+ };
+ };
+
};
pcie0_phy: phy@1c06000 {
@@ -1982,7 +2010,56 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
pinctrl-names = "default";
pinctrl-0 = <&pcie1_default_state>;
+ operating-points-v2 = <&pcie1_opp_table>;
+
status = "disabled";
+
+ pcie1_opp_table: opp-table {
+ compatible = "operating-points-v2";
+
+ /* GEN 1x1 */
+ opp-2500000 {
+ opp-hz = /bits/ 64 <2500000>;
+ required-opps = <&rpmhpd_opp_low_svs>;
+ opp-peak-kBps = <250000 1>;
+ };
+
+ /* GEN 1x2 GEN 2x1 */
+ opp-5000000 {
+ opp-hz = /bits/ 64 <5000000>;
+ required-opps = <&rpmhpd_opp_low_svs>;
+ opp-peak-kBps = <500000 1>;
+ };
+
+ /* GEN 2x2 */
+ opp-10000000 {
+ opp-hz = /bits/ 64 <10000000>;
+ required-opps = <&rpmhpd_opp_low_svs>;
+ opp-peak-kBps = <1000000 1>;
+ };
+
+ /* GEN 3x1 */
+ opp-8000000 {
+ opp-hz = /bits/ 64 <8000000>;
+ required-opps = <&rpmhpd_opp_nom>;
+ opp-peak-kBps = <984500 1>;
+ };
+
+ /* GEN 3x2 GEN 4x1 */
+ opp-16000000 {
+ opp-hz = /bits/ 64 <16000000>;
+ required-opps = <&rpmhpd_opp_nom>;
+ opp-peak-kBps = <1969000 1>;
+ };
+
+ /* GEN 4x2 */
+ opp-32000000 {
+ opp-hz = /bits/ 64 <32000000>;
+ required-opps = <&rpmhpd_opp_nom>;
+ opp-peak-kBps = <3938000 1>;
+ };
+ };
+
};
pcie1_phy: phy@1c0e000 {
--
2.42.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox