* [GIT PULL 0/4] ARM: SoC branches for v5.8
From: Arnd Bergmann @ 2020-06-04 20:45 UTC (permalink / raw)
To: Linus Torvalds, SoC Team, Linux ARM, Linux Kernel Mailing List,
Andreas Färber, Marek Vasut, Geert Uytterhoeven, Rob Herring,
Anson Huang, Doug Anderson
Hi Linus,
Here are the usual branches for arm-soc: five new SoC variants, 25
machines added, two machines removed, a few new drivers and the
usual amount of of cleanups and rework.
There are 835 patches from 196 contributors in total, with only
these contributing more than ten patches:
36 Marek Vasut
35 Andreas Färber
27 Geert Uytterhoeven
23 Rob Herring
22 Anson Huang
21 Douglas Anderson
20 Tudor Ambarus
20 Lad Prabhakar
19 Johan Jonker
17 Jonathan Bakker
17 Andre Przywara
16 Suman Anna
15 Jerome Brunet
14 Serge Semin
14 Bjorn Andersson
13 Tony Lindgren
12 Lubomir Rintel
12 Dmitry Osipenko
11 Sudeep Holla
11 Marek Szyprowski
The dirstat shows that as usual most changes are for device
tree files, this time slightly more 32-bit than 64-bit both in number
and size of the changes, while 60% of the newly added machines
are 64-bit.
0.3% Documentation/devicetree/bindings/arm/
0.6% Documentation/devicetree/bindings/bus/
0.2% Documentation/devicetree/bindings/display/tegra/
0.3% Documentation/devicetree/bindings/memory-controllers/
0.5% Documentation/devicetree/bindings/
31.9% arch/arm/boot/dts/
0.7% arch/arm/configs/
0.5% arch/arm/mach-imx/
1.2% arch/arm/mach-integrator/
3.2% arch/arm/mach-omap2/
5.4% arch/arm/mach-pxa/
0.5% arch/arm/
0.6% arch/arm64/boot/dts/allwinner/
4.0% arch/arm64/boot/dts/amlogic/
1.5% arch/arm64/boot/dts/arm/
2.4% arch/arm64/boot/dts/freescale/
0.2% arch/arm64/boot/dts/hisilicon/
5.2% arch/arm64/boot/dts/mediatek/
0.4% arch/arm64/boot/dts/nvidia/
7.6% arch/arm64/boot/dts/qcom/
1.7% arch/arm64/boot/dts/realtek/
2.0% arch/arm64/boot/dts/renesas/
1.4% arch/arm64/boot/dts/rockchip/
0.3% arch/arm64/boot/dts/socionext/
0.4% arch/arm64/boot/dts/sprd/
0.5% arch/arm64/boot/dts/ti/
2.8% drivers/bus/
0.9% drivers/clk/mediatek/
0.4% drivers/clk/versatile/
0.6% drivers/cpufreq/
0.7% drivers/firmware/arm_scmi/
0.2% drivers/firmware/
1.1% drivers/gpu/drm/mediatek/
0.6% drivers/memory/
0.2% drivers/mfd/
0.6% drivers/misc/
0.4% drivers/reset/
0.3% drivers/soc/amlogic/
0.3% drivers/soc/imx/
1.1% drivers/soc/mediatek/
3.0% drivers/soc/qcom/
0.2% drivers/soc/tegra/fuse/
0.3% drivers/soc/ti/
0.2% drivers/soc/
7.8% drivers/staging/media/tegra-video/
0.4% drivers/tee/
0.2% include/dt-bindings/clock/
0.2% include/dt-bindings/firmware/imx/
0.6% include/dt-bindings/reset/
0.3% include/linux/
0.3% include/
The largest outlier are a few added drivers for the Tegra platform
and on MIPS SoC (Baikal-T1), for which I helped get some drivers
reviewed and merged. The changes for omap2 and pxa are
cleanups moving code out of the platform directory.
As of this morning, there were no merge conflicts against your tree.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v2] iommu/arm-smmu: Mark qcom_smmu_client_of_match as possibly unused
From: Jordan Crouse @ 2020-06-04 20:39 UTC (permalink / raw)
To: linux-arm-msm
Cc: Sai Prakash Ranjan, Robin Murphy, Joerg Roedel, linux-kernel,
iommu, Will Deacon, linux-arm-kernel
When CONFIG_OF=n of_match_device() gets pre-processed out of existence
leaving qcom-smmu_client_of_match unused. Mark it as possibly unused to
keep the compiler from warning in that case.
Fixes: 0e764a01015d ("iommu/arm-smmu: Allow client devices to select direct mapping")
Reported-by: kbuild test robot <lkp@intel.com>
Acked-by: Will Deacon <will@kernel.org>
Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---
drivers/iommu/arm-smmu-qcom.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/arm-smmu-qcom.c b/drivers/iommu/arm-smmu-qcom.c
index cf01d0215a39..be4318044f96 100644
--- a/drivers/iommu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm-smmu-qcom.c
@@ -12,7 +12,7 @@ struct qcom_smmu {
struct arm_smmu_device smmu;
};
-static const struct of_device_id qcom_smmu_client_of_match[] = {
+static const struct of_device_id qcom_smmu_client_of_match[] __maybe_unused = {
{ .compatible = "qcom,adreno" },
{ .compatible = "qcom,mdp4" },
{ .compatible = "qcom,mdss" },
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH 3/3] spi: bcm2835: Enable shared interrupt support
From: Florian Fainelli @ 2020-06-04 20:24 UTC (permalink / raw)
To: Mark Brown
Cc: moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Scott Branden, lukas, Ray Jui, linux-kernel,
open list:SPI SUBSYSTEM, Rob Herring,
maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Martin Sperl, Nicolas Saenz Julienne
In-Reply-To: <21772111-fa1f-7a50-aa92-e44b09cff4eb@gmail.com>
On 6/4/2020 9:05 AM, Florian Fainelli wrote:
>
>
> On 6/4/2020 5:32 AM, Mark Brown wrote:
>> On Wed, Jun 03, 2020 at 08:46:55PM -0700, Florian Fainelli wrote:
>>> The SPI controller found in the BCM2711 and BCM7211 SoCs is instantiated
>>> 5 times, with all instances sharing the same interrupt line. We
>>> specifically match the two compatible strings here to determine whether
>>> it is necessary to request the interrupt with the IRQF_SHARED flag and
>>> to use an appropriate interrupt handler capable of returning IRQ_NONE.
>>
>>> For the BCM2835 case which is deemed performance critical, there is no
>>> overhead since a dedicated handler that does not assume sharing is used.
>>
>> This feels hacky - it's essentially using the compatible string to set a
>> boolean flag which isn't really about the IP but rather the platform
>> integration. It might cause problems if we do end up having to quirk
>> this version of the IP for some other reason.
>
> I am not sure why it would be a problem, when you describe a piece of
> hardware with Device Tree, even with the IP block being strictly the
> same, its very integration into a new SoC (with details like shared
> interrupt lines) do warrant a different compatible string. Maybe this is
> more of a philosophical question.
>
>> I'm also looking at the
>> code and wondering if the overhead of checking to see if the interrupt
>> is flagged is really that severe, it's just a check to see if a bit is
>> set in a register which we already read so should be a couple of
>> instructions (which disassembly seems to confirm). It *is* overhead so
>> there's some value in it, I'm just surprised that it's such a hot path
>> especially with a reasonably deep FIFO like this device has.
>
> If it was up to me, we would just add the check on BCM2835_SPI_CS_INTR
> not being set and return IRQ_NONE and be done with it. I appreciate that
> Lukas has spent some tremendous amount of time working on this
> controller driver and he has a sensitivity for performance.
>
>>
>> I guess ideally genirq would provide a way to figure out if an interrupt
>> is actually shared in the present system, and better yet we'd have a way
>> for drivers to say they aren't using the interrupt ATM, but that might
>> be more effort than it's really worth. If this is needed and there's no
>> better way of figuring out if the interrupt is really shared then I'd
>> suggest a boolean flag rather than a compatible string, it's still a
>> hack but it's less likely to store up trouble for the future.
>
> Instead of counting the number of SPI devices we culd request the
> interrupt first with flags = IRQF_PROBE_SHARED, if this works, good we
> have a single SPI master enabled, if it returns -EBUSY, try again with
> flags = IRQF_SHARED and set-up the bcm2835_spi_sh_interrupt interrupt
> handler to manage the sharing.
>
> This would not require DT changes, which is probably better anyway.
Unfortunately this does not work.. The first time we probe the driver we
need to set an interrupt handler that is capable of handling a shared
interrupt. When we probe for subsequent times, we can use the -EBUSY
return code to know that we are in a shared interrupt context, however,
it is too late to change the first controller interrupt handler.
So we do need to know for the first time we install the interrupt
handler whether we will be in a shared situation or not, I cannot think
of any solution other than counting the number of available DT nodes
with the "brcm,bcm2835-spi" compatible string.
--
Florian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 3/3] media: cedrus: h264: Fix frame list construction
From: Jernej Skrabec @ 2020-06-04 18:57 UTC (permalink / raw)
To: paul.kocialkowski, mripard
Cc: devel, jonas, gregkh, linux-kernel, nicolas, wens, hverkuil-cisco,
mchehab, linux-arm-kernel, linux-media
In-Reply-To: <20200604185745.23568-1-jernej.skrabec@siol.net>
Current frame list construction algorithm assumes that decoded image
will be output into its own buffer. That is true for progressive content
but not for interlaced where each field is decoded separately into same
buffer.
Fix that by checking if capture buffer is listed in DPB. If it is, reuse
it.
Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
index c87717d17ec5..4f79386315ae 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
@@ -102,7 +102,7 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx,
struct cedrus_dev *dev = ctx->dev;
unsigned long used_dpbs = 0;
unsigned int position;
- unsigned int output = 0;
+ int output = -1;
unsigned int i;
cap_q = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
@@ -125,6 +125,11 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx,
position = cedrus_buf->codec.h264.position;
used_dpbs |= BIT(position);
+ if (run->dst->vb2_buf.timestamp == dpb->reference_ts) {
+ output = position;
+ continue;
+ }
+
if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
continue;
@@ -132,13 +137,11 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx,
dpb->top_field_order_cnt,
dpb->bottom_field_order_cnt,
&pic_list[position]);
-
- output = max(position, output);
}
- position = find_next_zero_bit(&used_dpbs, CEDRUS_H264_FRAME_NUM,
- output);
- if (position >= CEDRUS_H264_FRAME_NUM)
+ if (output >= 0)
+ position = output;
+ else
position = find_first_zero_bit(&used_dpbs, CEDRUS_H264_FRAME_NUM);
output_buf = vb2_to_cedrus_buffer(&run->dst->vb2_buf);
--
2.27.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 2/3] media: cedrus: h264: Properly configure reference field
From: Jernej Skrabec @ 2020-06-04 18:57 UTC (permalink / raw)
To: paul.kocialkowski, mripard
Cc: devel, jonas, gregkh, linux-kernel, nicolas, wens, hverkuil-cisco,
mchehab, linux-arm-kernel, linux-media
In-Reply-To: <20200604185745.23568-1-jernej.skrabec@siol.net>
When interlaced H264 content is being decoded, references must indicate
which field is being referenced. Currently this was done by checking
capture buffer flags. However, that is not correct because capture
buffer may hold both fields.
Fix this by checking newly introduced flags in reference lists.
Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
index cce527bbdf86..c87717d17ec5 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
@@ -183,7 +183,6 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
for (i = 0; i < num_ref; i++) {
const struct v4l2_h264_dpb_entry *dpb;
const struct cedrus_buffer *cedrus_buf;
- const struct vb2_v4l2_buffer *ref_buf;
unsigned int position;
int buf_idx;
u8 dpb_idx;
@@ -198,12 +197,11 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
if (buf_idx < 0)
continue;
- ref_buf = to_vb2_v4l2_buffer(cap_q->bufs[buf_idx]);
- cedrus_buf = vb2_v4l2_to_cedrus_buffer(ref_buf);
+ cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);
position = cedrus_buf->codec.h264.position;
sram_array[i] |= position << 1;
- if (ref_buf->field == V4L2_FIELD_BOTTOM)
+ if (ref_list[i].flags & V4L2_H264_REFERENCE_FLAG_BOTTOM_FIELD)
sram_array[i] |= BIT(0);
}
--
2.27.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 1/3] media: uapi: h264: update reference lists
From: Jernej Skrabec @ 2020-06-04 18:57 UTC (permalink / raw)
To: paul.kocialkowski, mripard
Cc: devel, jonas, gregkh, linux-kernel, nicolas, wens, hverkuil-cisco,
mchehab, linux-arm-kernel, linux-media
In-Reply-To: <20200604185745.23568-1-jernej.skrabec@siol.net>
When dealing with with interlaced frames, reference lists must tell if
each particular reference is meant for top or bottom field. This info
is currently not provided at all in the H264 related controls.
Make reference lists hold a structure which will also hold flags along
index into DPB array. Flags will tell if reference is meant for top or
bottom field.
Currently the only user of these lists is Cedrus which is just compile
fixed here. Actual usage of newly introduced flags will come in
following commit.
Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
.../media/v4l/ext-ctrls-codec.rst | 40 ++++++++++++++++++-
.../staging/media/sunxi/cedrus/cedrus_h264.c | 6 +--
include/media/h264-ctrls.h | 12 +++++-
3 files changed, 51 insertions(+), 7 deletions(-)
diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index d0d506a444b1..6c36d298db20 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -1843,10 +1843,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
* - __u32
- ``slice_group_change_cycle``
-
- * - __u8
+ * - struct :c:type:`v4l2_h264_reference`
- ``ref_pic_list0[32]``
- Reference picture list after applying the per-slice modifications
- * - __u8
+ * - struct :c:type:`v4l2_h264_reference`
- ``ref_pic_list1[32]``
- Reference picture list after applying the per-slice modifications
* - __u32
@@ -1926,6 +1926,42 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
- ``chroma_offset[32][2]``
-
+``Picture Reference``
+
+.. c:type:: v4l2_h264_reference
+
+.. cssclass:: longtable
+
+.. flat-table:: struct v4l2_h264_reference
+ :header-rows: 0
+ :stub-columns: 0
+ :widths: 1 1 2
+
+ * - __u16
+ - ``flags``
+ - See :ref:`Picture Reference Flags <h264_reference_flags>`
+ * - __u8
+ - ``index``
+ -
+
+.. _h264_reference_flags:
+
+``Picture Reference Flags``
+
+.. cssclass:: longtable
+
+.. flat-table::
+ :header-rows: 0
+ :stub-columns: 0
+ :widths: 1 1 2
+
+ * - ``V4L2_H264_REFERENCE_FLAG_TOP_FIELD``
+ - 0x00000001
+ -
+ * - ``V4L2_H264_REFERENCE_FLAG_BOTTOM_FIELD``
+ - 0x00000002
+ -
+
``V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS (struct)``
Specifies the decode parameters (as extracted from the bitstream)
for the associated H264 slice data. This includes the necessary
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
index 54ee2aa423e2..cce527bbdf86 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
@@ -166,8 +166,8 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx,
static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
struct cedrus_run *run,
- const u8 *ref_list, u8 num_ref,
- enum cedrus_h264_sram_off sram)
+ const struct v4l2_h264_reference *ref_list,
+ u8 num_ref, enum cedrus_h264_sram_off sram)
{
const struct v4l2_ctrl_h264_decode_params *decode = run->h264.decode_params;
struct vb2_queue *cap_q;
@@ -188,7 +188,7 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
int buf_idx;
u8 dpb_idx;
- dpb_idx = ref_list[i];
+ dpb_idx = ref_list[i].index;
dpb = &decode->dpb[dpb_idx];
if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
index 080fd1293c42..9b1cbc9bc38e 100644
--- a/include/media/h264-ctrls.h
+++ b/include/media/h264-ctrls.h
@@ -140,6 +140,14 @@ struct v4l2_h264_pred_weight_table {
#define V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED 0x04
#define V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH 0x08
+#define V4L2_H264_REFERENCE_FLAG_TOP_FIELD 0x01
+#define V4L2_H264_REFERENCE_FLAG_BOTTOM_FIELD 0x02
+
+struct v4l2_h264_reference {
+ __u8 flags;
+ __u8 index;
+};
+
struct v4l2_ctrl_h264_slice_params {
/* Size in bytes, including header */
__u32 size;
@@ -182,8 +190,8 @@ struct v4l2_ctrl_h264_slice_params {
* Entries on each list are indices into
* v4l2_ctrl_h264_decode_params.dpb[].
*/
- __u8 ref_pic_list0[32];
- __u8 ref_pic_list1[32];
+ struct v4l2_h264_reference ref_pic_list0[32];
+ struct v4l2_h264_reference ref_pic_list1[32];
__u32 flags;
};
--
2.27.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 0/3] media: uapi: cedrus: Fix decoding interlaced H264 content
From: Jernej Skrabec @ 2020-06-04 18:57 UTC (permalink / raw)
To: paul.kocialkowski, mripard
Cc: devel, jonas, gregkh, linux-kernel, nicolas, wens, hverkuil-cisco,
mchehab, linux-arm-kernel, linux-media
Currently H264 interlaced content it's not properly decoded on Cedrus.
There are two reasons for this:
1. slice parameters control doesn't provide enough information
2. bug in frame list construction in Cedrus driver
As described in commit message in patch 1, references stored in
reference lists should tell if reference targets top or bottom field.
However, this information is currently not provided. Patch 1 adds
it in form of flags which are set for each reference. Patch 2 then
uses those flags in Cedrus driver.
Frame list construction is fixed in patch 3.
This solution was extensively tested using Kodi on LibreELEC with A64,
H3, H5 and H6 SoCs in slightly different form (flags were transmitted
in MSB bits in index).
Note: I'm not 100% sure if flags for both, top and bottom fields are
needed. Any input here would be welcome.
Please take a look.
Best regards,
Jernej
Jernej Skrabec (3):
media: uapi: h264: update reference lists
media: cedrus: h264: Properly configure reference field
media: cedrus: h264: Fix frame list construction
.../media/v4l/ext-ctrls-codec.rst | 40 ++++++++++++++++++-
.../staging/media/sunxi/cedrus/cedrus_h264.c | 27 +++++++------
include/media/h264-ctrls.h | 12 +++++-
3 files changed, 62 insertions(+), 17 deletions(-)
--
2.27.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver
From: Tomasz Figa @ 2020-06-04 18:05 UTC (permalink / raw)
To: Sakari Ailus
Cc: Mark Rutland, Nicolas Boichat, Andy Shevchenko, srv_heupstream,
linux-devicetree, Linus Walleij,
Shengnan Wang (王圣男), Bartosz Golaszewski,
Sj Huang, Rob Herring, moderated list:ARM/Mediatek SoC support,
Dongchun Zhu, Louis Kuo, Matthias Brugger, Cao Bing Bu,
Mauro Carvalho Chehab,
list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>, ,
Linux Media Mailing List
In-Reply-To: <20200604092616.GJ16711@paasikivi.fi.intel.com>
On Thu, Jun 4, 2020 at 11:26 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Dongchun,
>
> On Thu, Jun 04, 2020 at 10:14:05AM +0800, Dongchun Zhu wrote:
> > Hi Tomasz, Sakari, and sirs,
> >
> > Could anyone help to review this patch?
> >
> > On Sat, 2020-05-23 at 16:41 +0800, Dongchun Zhu wrote:
> > > Add a V4L2 sub-device driver for OV02A10 image sensor.
> > >
> > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > ---
> > > MAINTAINERS | 1 +
> > > drivers/media/i2c/Kconfig | 13 +
> > > drivers/media/i2c/Makefile | 1 +
> > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++
> > > 4 files changed, 1040 insertions(+)
> > > create mode 100644 drivers/media/i2c/ov02a10.c
> > >
> >
> > [snip]
> >
> > > +static int ov02a10_probe(struct i2c_client *client)
> > > +{
> > > + struct device *dev = &client->dev;
> > > + struct ov02a10 *ov02a10;
> > > + unsigned int rotation;
> > > + unsigned int clock_lane_tx_speed;
> > > + unsigned int i;
> > > + int ret;
> > > +
> > > + ov02a10 = devm_kzalloc(dev, sizeof(*ov02a10), GFP_KERNEL);
> > > + if (!ov02a10)
> > > + return -ENOMEM;
> > > +
> > > + ret = ov02a10_check_hwcfg(dev, ov02a10);
> > > + if (ret) {
> > > + dev_err(dev, "failed to check HW configuration: %d", ret);
> > > + return ret;
> > > + }
> > > +
> > > + v4l2_i2c_subdev_init(&ov02a10->subdev, client, &ov02a10_subdev_ops);
> > > + ov02a10->mipi_clock_tx_speed = OV02A10_MIPI_TX_SPEED_DEFAULT;
> > > + ov02a10->fmt.code = MEDIA_BUS_FMT_SBGGR10_1X10;
> > > +
> > > + /* Optional indication of physical rotation of sensor */
> > > + ret = fwnode_property_read_u32(dev_fwnode(dev), "rotation", &rotation);
> > > + if (!ret && rotation == 180) {
> > > + ov02a10->upside_down = true;
> > > + ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10;
> > > + }
> > > +
> > > + /* Optional indication of mipi TX speed */
> > > + ret = fwnode_property_read_u32(dev_fwnode(dev), "ovti,mipi-tx-speed",
> > > + &clock_lane_tx_speed);
> > > +
> > > + if (!ret)
> > > + ov02a10->mipi_clock_tx_speed = clock_lane_tx_speed;
> > > +
> > > + /* Get system clock (eclk) */
> > > + ov02a10->eclk = devm_clk_get(dev, "eclk");
> > > + if (IS_ERR(ov02a10->eclk)) {
> > > + ret = PTR_ERR(ov02a10->eclk);
> > > + dev_err(dev, "failed to get eclk %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
> > > + &ov02a10->eclk_freq);
> > > + if (ret) {
> > > + dev_err(dev, "failed to get eclk frequency\n");
> > > + return ret;
> > > + }
> > > +
> > > + ret = clk_set_rate(ov02a10->eclk, ov02a10->eclk_freq);
> > > + if (ret) {
> > > + dev_err(dev, "failed to set eclk frequency (24MHz)\n");
> > > + return ret;
> > > + }
> > > +
> > > + if (clk_get_rate(ov02a10->eclk) != OV02A10_ECLK_FREQ) {
> > > + dev_warn(dev, "wrong eclk frequency %d Hz, expected: %d Hz\n",
> > > + ov02a10->eclk_freq, OV02A10_ECLK_FREQ);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + ov02a10->pd_gpio = devm_gpiod_get(dev, "powerdown", GPIOD_OUT_HIGH);
> > > + if (IS_ERR(ov02a10->pd_gpio)) {
> > > + ret = PTR_ERR(ov02a10->pd_gpio);
> > > + dev_err(dev, "failed to get powerdown-gpios %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + ov02a10->n_rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> > > + if (IS_ERR(ov02a10->n_rst_gpio)) {
> > > + ret = PTR_ERR(ov02a10->n_rst_gpio);
> > > + dev_err(dev, "failed to get reset-gpios %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(ov02a10_supply_names); i++)
> > > + ov02a10->supplies[i].supply = ov02a10_supply_names[i];
> > > +
> > > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ov02a10_supply_names),
> > > + ov02a10->supplies);
> > > + if (ret) {
> > > + dev_err(dev, "failed to get regulators\n");
> > > + return ret;
> > > + }
> > > +
> > > + mutex_init(&ov02a10->mutex);
> > > + ov02a10->cur_mode = &supported_modes[0];
> > > + ret = ov02a10_initialize_controls(ov02a10);
> > > + if (ret) {
> > > + dev_err(dev, "failed to initialize controls\n");
> > > + goto err_destroy_mutex;
> > > + }
> > > +
> > > + ov02a10->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > + ov02a10->subdev.entity.ops = &ov02a10_subdev_entity_ops;
> > > + ov02a10->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > > + ov02a10->pad.flags = MEDIA_PAD_FL_SOURCE;
> > > + ret = media_entity_pads_init(&ov02a10->subdev.entity, 1, &ov02a10->pad);
> > > + if (ret < 0) {
> > > + dev_err(dev, "failed to init entity pads: %d", ret);
> > > + goto err_free_handler;
> > > + }
> > > +
> > > + pm_runtime_enable(dev);
> > > + if (!pm_runtime_enabled(dev)) {
> > > + ret = ov02a10_power_on(dev);
> > > + if (ret < 0) {
> > > + dev_err(dev, "failed to power on: %d\n", ret);
> > > + goto err_free_handler;
> > > + }
> > > + }
> > > +
> > > + ret = v4l2_async_register_subdev(&ov02a10->subdev);
> > > + if (ret) {
> > > + dev_err(dev, "failed to register V4L2 subdev: %d", ret);
> > > + if (!pm_runtime_enabled(dev))
> > > + ov02a10_power_off(dev);
>
> This should be moved to error handling section below.
>
> > > + goto err_clean_entity;
> > > + }
> >
> > Tomasz, Sakari, is this ok?
> > or coding like this:
> >
> > ret = v4l2_async_register_subdev(&ov02a10->subdev);
> > if (!pm_runtime_enabled(dev))
> > ov02a10_power_off(dev);
> > if (ret) {
> > dev_err(dev, "failed to register V4L2 subdev: %d", ret);
> > goto err_clean_entity;
> > }
> >
> > What's your opinions about the change?
>
> This turns power off if runtime PM is disabled. I'd keep it as-is, as it'd
> require re-implementing what runtime PM is used for now --- and that's not
> a sensor driver's job.
That and in general I believe the expectations are:
- runtime PM enabled - powered on only when it has something to do
- runtime PM disabled - powered on when the driver is bound (probe
succeeded), powered off when the driver unbinds (remove or probe
error)
Best regards,
Tomasz
>
> >
> > > +
> > > + return 0;
> > > +
> > > +err_clean_entity:
> > > + media_entity_cleanup(&ov02a10->subdev.entity);
> > > +err_free_handler:
> > > + v4l2_ctrl_handler_free(ov02a10->subdev.ctrl_handler);
> > > +err_destroy_mutex:
> > > + mutex_destroy(&ov02a10->mutex);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int ov02a10_remove(struct i2c_client *client)
> > > +{
> > > + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> > > +
> > > + v4l2_async_unregister_subdev(sd);
> > > + media_entity_cleanup(&sd->entity);
> > > + v4l2_ctrl_handler_free(sd->ctrl_handler);
> > > + pm_runtime_disable(&client->dev);
> > > + if (!pm_runtime_status_suspended(&client->dev))
> > > + ov02a10_power_off(&client->dev);
> > > + pm_runtime_set_suspended(&client->dev);
> > > + mutex_destroy(&ov02a10->mutex);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct of_device_id ov02a10_of_match[] = {
> > > + { .compatible = "ovti,ov02a10" },
> > > + {}
> > > +};
> > > +MODULE_DEVICE_TABLE(of, ov02a10_of_match);
> > > +
> > > +static struct i2c_driver ov02a10_i2c_driver = {
> > > + .driver = {
> > > + .name = "ov02a10",
> > > + .pm = &ov02a10_pm_ops,
> > > + .of_match_table = ov02a10_of_match,
> > > + },
> > > + .probe_new = &ov02a10_probe,
> > > + .remove = &ov02a10_remove,
> > > +};
> > > +
> > > +module_i2c_driver(ov02a10_i2c_driver);
> > > +
> > > +MODULE_AUTHOR("Dongchun Zhu <dongchun.zhu@mediatek.com>");
> > > +MODULE_DESCRIPTION("OmniVision OV02A10 sensor driver");
> > > +MODULE_LICENSE("GPL v2");
> > > +
> >
>
> --
> Sakari Ailus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken
From: Joe Perches @ 2020-06-04 18:02 UTC (permalink / raw)
To: Dan Carpenter
Cc: Linus Walleij, kernel-janitors, linux-kernel@vger.kernel.org,
Haojian Zhuang, Julia Lawall, open list:GPIO SUBSYSTEM,
Christophe JAILLET, Linux ARM, Robert Jarzmik, Daniel Mack
In-Reply-To: <20200604173500.GI22511@kadam>
On Thu, 2020-06-04 at 20:35 +0300, Dan Carpenter wrote:
> On Thu, Jun 04, 2020 at 09:08:44AM -0700, Joe Perches wrote:
> > On Thu, 2020-06-04 at 15:30 +0300, Dan Carpenter wrote:
> > > On Thu, Jun 04, 2020 at 01:42:12PM +0200, Julia Lawall wrote:
> > > > OK, I recall a discussion with Dan where he suggested that some things
> > > > that were not actually bug fixes could also merit a Fixes tag. But it's
> > > > probably better if he weighs in directly.
> > >
> > > I generally think Fixes should only be used for "real bug" fixes.
> > >
> > > The one exception is when I'm reviewing a patch that fixes an "unused
> > > assignment" static checker warning is that I know which commit
> > > introduced the warning.
Sometimes those warnings are introduced by new compiler
versions.
That's why I don't care for -Werror use in Makefiles.
> > > I don't have strong feelings if it's in the
> > > Fixes tag or if it's just mentioned in the commit message.
> >
> > My view is that changes that silence compiler warnings are
> > not fixing bugs and that these changes should generally not
> > be backported.
> >
> The Fixes tag is useful for backports but that's not whole the point of
> it. It's also for collecting metrics.
Hmm, how are these metrics used?
> Also sometimes we fix the bug
> before the kernel is released so the Fixes tag means we can automatically
> ignore those ones when we look at which patches to backport.
>
> I don't care if the "unused assignment" patches use a Fixes tag or just
> mention the commit. Either way the information is there for when I
> review the patch.
Perhaps there could/should be some distinction between
"real bug" fixes and trivialities like "unused assignment"
Maybe something like:
Updates: <commit> ("commit description")
vs
Fixes: <commit> ("commit description")
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 12/25] clk: bcm: rpi: Use CCF boundaries instead of rolling our own
From: Nicolas Saenz Julienne @ 2020-06-04 18:02 UTC (permalink / raw)
To: Maxime Ripard
Cc: Tim Gover, Dave Stevenson, linux-kernel, bcm-kernel-feedback-list,
linux-rpi-kernel, Phil Elwell, linux-arm-kernel
In-Reply-To: <eb1b2838f1c3c006c24bcb9816f75e1351c63b05.1590594293.git-series.maxime@cerno.tech>
[-- Attachment #1.1: Type: text/plain, Size: 2411 bytes --]
On Wed, 2020-05-27 at 17:45 +0200, Maxime Ripard wrote:
> The raspberrypi firmware clock driver has a min_rate / max_rate clamping by
> storing the info it needs in a private structure.
>
> However, the CCF already provides such a facility, so we can switch to it
> to remove the boilerplate.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
> drivers/clk/bcm/clk-raspberrypi.c | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-
> raspberrypi.c
> index a20492fade6a..e135ad28d38d 100644
> --- a/drivers/clk/bcm/clk-raspberrypi.c
> +++ b/drivers/clk/bcm/clk-raspberrypi.c
> @@ -36,9 +36,6 @@ struct raspberrypi_clk {
> struct rpi_firmware *firmware;
> struct platform_device *cpufreq;
>
> - unsigned long min_rate;
> - unsigned long max_rate;
> -
> struct clk_hw pllb;
> };
>
> @@ -142,13 +139,11 @@ static int raspberrypi_fw_pll_set_rate(struct clk_hw
> *hw, unsigned long rate,
> static int raspberrypi_pll_determine_rate(struct clk_hw *hw,
> struct clk_rate_request *req)
> {
> - struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk,
> - pllb);
> u64 div, final_rate;
> u32 ndiv, fdiv;
>
> /* We can't use req->rate directly as it would overflow */
> - final_rate = clamp(req->rate, rpi->min_rate, rpi->max_rate);
> + final_rate = clamp(req->rate, req->min_rate, req->max_rate);
>
> div = (u64)final_rate << A2W_PLL_FRAC_BITS;
> do_div(div, req->best_parent_rate);
> @@ -215,12 +210,15 @@ static int raspberrypi_register_pllb(struct
> raspberrypi_clk *rpi)
> dev_info(rpi->dev, "CPU frequency range: min %u, max %u\n",
> min_rate, max_rate);
>
> - rpi->min_rate = min_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE;
> - rpi->max_rate = max_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE;
> -
> rpi->pllb.init = &init;
>
> - return devm_clk_hw_register(rpi->dev, &rpi->pllb);
> + ret = devm_clk_hw_register(rpi->dev, &rpi->pllb);
> + if (!ret)
> + clk_hw_set_rate_range(&rpi->pllb,
> + min_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE,
> + max_rate *
> RPI_FIRMWARE_PLLB_ARM_DIV_RATE);
Isn't there a potential race here? Albeit unlikely, cpufreq could show up and
call clk_round_rate() in between the registration and you setting the ranges.
Regards,
Nicolas
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 04/25] clk: bcm: rpi: Allow the driver to be probed by DT
From: Nicolas Saenz Julienne @ 2020-06-04 17:52 UTC (permalink / raw)
To: Maxime Ripard
Cc: Tim Gover, Dave Stevenson, Stephen Boyd, Michael Turquette,
linux-kernel, linux-clk, bcm-kernel-feedback-list,
linux-rpi-kernel, Phil Elwell, linux-arm-kernel
In-Reply-To: <fa709f71b27aadf987685f7cae2a65cc3cef8e3d.1590594293.git-series.maxime@cerno.tech>
[-- Attachment #1.1: Type: text/plain, Size: 867 bytes --]
On Wed, 2020-05-27 at 17:45 +0200, Maxime Ripard wrote:
> The current firmware clock driver for the RaspberryPi can only be probed by
> manually registering an associated platform_device.
>
> While this works fine for cpufreq where the device gets attached a clkdev
> lookup, it would be tedious to maintain a table of all the devices using
> one of the clocks exposed by the firmware.
>
> Since the DT on the other hand is the perfect place to store those
> associations, make the firmware clocks driver probe-able through the device
> tree so that we can represent it as a node.
>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
Reviewed-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
Regards,
Nicolas
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 03/25] firmware: rpi: Only create clocks device if we don't have a node for it
From: Nicolas Saenz Julienne @ 2020-06-04 17:50 UTC (permalink / raw)
To: Maxime Ripard
Cc: Tim Gover, Dave Stevenson, linux-kernel, bcm-kernel-feedback-list,
linux-rpi-kernel, Phil Elwell, linux-arm-kernel
In-Reply-To: <f0b979221dedec7db9cab201388bf44c0d987a3d.1590594293.git-series.maxime@cerno.tech>
[-- Attachment #1.1: Type: text/plain, Size: 1463 bytes --]
On Wed, 2020-05-27 at 17:44 +0200, Maxime Ripard wrote:
> The firmware clocks driver was previously probed through a platform_device
> created by the firmware driver.
>
> Since we will now have a node for that clocks driver, we need to create the
> device only in the case where there's no node for it already.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
Reviewed-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
Regards,
Nicolas
> drivers/firmware/raspberrypi.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
> index ef8098856a47..b25901a77c09 100644
> --- a/drivers/firmware/raspberrypi.c
> +++ b/drivers/firmware/raspberrypi.c
> @@ -208,6 +208,20 @@ rpi_register_hwmon_driver(struct device *dev, struct
> rpi_firmware *fw)
>
> static void rpi_register_clk_driver(struct device *dev)
> {
> + struct device_node *firmware;
> +
> + /*
> + * Earlier DTs don't have a node for the firmware clocks but
> + * rely on us creating a platform device by hand. If we do
> + * have a node for the firmware clocks, just bail out here.
> + */
> + firmware = of_get_compatible_child(dev->of_node,
> + "raspberrypi,firmware-clocks");
> + if (firmware) {
> + of_node_put(firmware);
> + return;
> + }
> +
> rpi_clk = platform_device_register_data(dev, "raspberrypi-clk",
> -1, NULL, 0);
> }
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken
From: Dan Carpenter @ 2020-06-04 17:35 UTC (permalink / raw)
To: Joe Perches
Cc: Linus Walleij, kernel-janitors, linux-kernel@vger.kernel.org,
Haojian Zhuang, Julia Lawall, open list:GPIO SUBSYSTEM,
Christophe JAILLET, Linux ARM, Robert Jarzmik, Daniel Mack
In-Reply-To: <0749ac5e3868c6ba50728ced8366bfd86b0b8500.camel@perches.com>
On Thu, Jun 04, 2020 at 09:08:44AM -0700, Joe Perches wrote:
> On Thu, 2020-06-04 at 15:30 +0300, Dan Carpenter wrote:
> > On Thu, Jun 04, 2020 at 01:42:12PM +0200, Julia Lawall wrote:
> > > OK, I recall a discussion with Dan where he suggested that some things
> > > that were not actually bug fixes could also merit a Fixes tag. But it's
> > > probably better if he weighs in directly.
> >
> > I generally think Fixes should only be used for "real bug" fixes.
> >
> > The one exception is when I'm reviewing a patch that fixes an "unused
> > assignment" static checker warning is that I know which commit
> > introduced the warning. I don't have strong feelings if it's in the
> > Fixes tag or if it's just mentioned in the commit message.
>
> My view is that changes that silence compiler warnings are
> not fixing bugs and that these changes should generally not
> be backported.
>
The Fixes tag is useful for backports but that's not whole the point of
it. It's also for collecting metrics. Also sometimes we fix the bug
before the kernel is released so the Fixes tag means we can automatically
ignore those ones when we look at which patches to backport.
I don't care if the "unused assignment" patches use a Fixes tag or just
mention the commit. Either way the information is there for when I
review the patch.
regards,
dan carpenter
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] iio: adc: mt6360: Add ADC driver for MT6360
From: Jonathan Cameron @ 2020-06-04 17:33 UTC (permalink / raw)
To: Gene Chen
Cc: gene_chen, lars, linux-iio, linux-kernel, cy_huang, benjamin.chao,
linux-mediatek, linux-arm-kernel, pmeerw, knaack.h, matthias.bgg,
Wilma.Wu, jic23, shufan_lee
In-Reply-To: <1591239631-12265-1-git-send-email-gene.chen.richtek@gmail.com>
On Thu, 4 Jun 2020 11:00:31 +0800
Gene Chen <gene.chen.richtek@gmail.com> wrote:
> From: Gene Chen <gene_chen@richtek.com>
>
> Add MT6360 ADC driver include Charger Current, Voltage, and
> Temperature.
>
> Signed-off-by: Gene Chen <gene_chen@richtek.com>
> base-commit: 098c4adf249c198519a4abebe482b1e6b8c50e47
Hi Gene,
Comments inline.
I'd like to understand more in particularly on why the thread, rather than
using one of the standard triggers that handles that for you?
(I can think of a few reasons but better to hear from you!)
Thanks,
Jonathan
> ---
> drivers/iio/adc/Kconfig | 11 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/mt6360-adc.c | 419 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 431 insertions(+)
> create mode 100644 drivers/iio/adc/mt6360-adc.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 12bb8b7..a9736ec 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -657,6 +657,17 @@ config MCP3911
> This driver can also be built as a module. If so, the module will be
> called mcp3911.
>
> +config MEDIATEK_MT6360_ADC
> + tristate "Mediatek MT6360 ADC Part"
> + depends on MFD_MT6360
> + select IIO_BUFFER
> + select IIO_KFIFO_BUF
> + help
> + Say Y here to enable MT6360 ADC Part.
> + Integrated for System Monitoring include
> + Charger and Battery Current, Voltage and
> + Terperature
Temperature
> +
> config MEDIATEK_MT6577_AUXADC
> tristate "MediaTek AUXADC driver"
> depends on ARCH_MEDIATEK || COMPILE_TEST
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 6378078..4209776 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -62,6 +62,7 @@ obj-$(CONFIG_MAX9611) += max9611.o
> obj-$(CONFIG_MCP320X) += mcp320x.o
> obj-$(CONFIG_MCP3422) += mcp3422.o
> obj-$(CONFIG_MCP3911) += mcp3911.o
> +obj-$(CONFIG_MEDIATEK_MT6360_ADC) += mt6360-adc.o
> obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
> obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
> obj-$(CONFIG_MESON_SARADC) += meson_saradc.o
> diff --git a/drivers/iio/adc/mt6360-adc.c b/drivers/iio/adc/mt6360-adc.c
> new file mode 100644
> index 0000000..bc9c488
> --- /dev/null
> +++ b/drivers/iio/adc/mt6360-adc.c
> @@ -0,0 +1,419 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020 MediaTek Inc.
> + *
> + * Author: Gene Chen <gene_chen@richtek.com>
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/kfifo_buf.h>
> +#include <linux/init.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/kthread.h>
> +#include <linux/ktime.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/mfd/mt6360.h>
> +
> +/* CHG_CTRL3 0x13 */
> +#define MT6360_AICR_MASK (0xFC)
> +#define MT6360_AICR_SHFT (2)
> +#define MT6360_AICR_400MA (0x6)
> +/* ADC_CONFIG 0x56 */
> +#define MT6360_ADCEN_SHFT (7)
> +/* ADC_RPT_1 0x5A */
> +#define MT6360_PREFERCH_MASK (0xF0)
> +#define MT6360_PREFERCH_SHFT (4)
> +#define MT6360_RPTCH_MASK (0x0F)
No need for brackets around raw numbers.
> +
> +enum {
> + MT6360_CHAN_USBID = 0,
> + MT6360_CHAN_VBUSDIV5,
> + MT6360_CHAN_VBUSDIV2,
> + MT6360_CHAN_VSYS,
> + MT6360_CHAN_VBAT,
> + MT6360_CHAN_IBUS,
> + MT6360_CHAN_IBAT,
> + MT6360_CHAN_CHG_VDDP,
> + MT6360_CHAN_TEMP_JC,
> + MT6360_CHAN_VREF_TS,
> + MT6360_CHAN_TS,
> + MT6360_CHAN_MAX,
> +};
> +
> +struct mt6360_adc_data {
> + struct device *dev;
> + struct regmap *regmap;
> + struct task_struct *scan_task;
> + struct completion adc_complete;
> + struct mutex adc_lock;
> + ktime_t last_off_timestamps[MT6360_CHAN_MAX];
> + int irq;
> +};
> +
> +static inline int mt6360_adc_val_converte(int val, int multiplier,
> + int offset, int divisor)
> +{
> + return ((val * multiplier) + offset) / divisor;
> +}
> +
> +static int mt6360_adc_get_process_val(struct mt6360_adc_data *info,
> + int chan_idx, int *val)
> +{
> + unsigned int regval = 0;
> + int ret;
> + const struct converter {
> + int multiplier;
> + int offset;
> + int divisor;
> + } adc_converter[MT6360_CHAN_MAX] = {
> + { 1250, 0, 1}, /* USBID */
> + { 6250, 0, 1}, /* VBUSDIV5 */
> + { 2500, 0, 1}, /* VBUSDIV2 */
> + { 1250, 0, 1}, /* VSYS */
> + { 1250, 0, 1}, /* VBAT */
> + { 2500, 0, 1}, /* IBUS */
> + { 2500, 0, 1}, /* IBAT */
> + { 1250, 0, 1}, /* CHG_VDDP */
> + { 105, -8000, 100}, /* TEMP_JC */
> + { 1250, 0, 1}, /* VREF_TS */
> + { 1250, 0, 1}, /* TS */
> + }, sp_ibus_adc_converter = { 1900, 0, 1 }, *sel_converter;
> +
> + if (chan_idx < 0 || chan_idx >= MT6360_CHAN_MAX)
> + return -EINVAL;
> + sel_converter = adc_converter + chan_idx;
> + if (chan_idx == MT6360_CHAN_IBUS) {
> + /* ibus chan will be affected by aicr config */
> + /* if aicr < 400, apply the special ibus converter */
> + ret = regmap_read(info->regmap, MT6360_PMU_CHG_CTRL3, ®val);
> + if (ret < 0)
> + return ret;
> + regval = (regval & MT6360_AICR_MASK) >> MT6360_AICR_SHFT;
> + if (regval < MT6360_AICR_400MA)
> + sel_converter = &sp_ibus_adc_converter;
> + }
> + *val = mt6360_adc_val_converte(*val, sel_converter->multiplier,
> + sel_converter->offset, sel_converter->divisor);
As mentioned below. Preference is always for linear conversion to be left
to consumers, either in userspace or when in kernel let the core code
deal with applying them.
So unless I'm missing something we should have offset and scale provided
via appropriate IIO_INFO elements and read_raw callbacks.
> + return 0;
> +}
> +
> +static int mt6360_adc_read_raw(struct iio_dev *iio_dev,
> + const struct iio_chan_spec *chan,
> + int *val, int *val2, long mask)
> +{
> + struct mt6360_adc_data *mad = iio_priv(iio_dev);
> + long timeout;
> + u8 tmp[2], rpt[3];
> + ktime_t start_t, predict_end_t;
> + int ret;
> +
> + mutex_lock(&mad->adc_lock);
> + /* select preferred channel that we want */
> + ret = regmap_update_bits(mad->regmap,
> + MT6360_PMU_ADC_RPT_1, MT6360_PREFERCH_MASK,
> + chan->channel << MT6360_PREFERCH_SHFT);
> + if (ret < 0)
> + goto err_adc_init;
Blank line here would help readability a tiny bit.
Same in other cases where you have a statement then an error handling block.
> + /* enable adc channel we want and adc_en */
> + memset(tmp, 0, sizeof(tmp));
> + tmp[0] |= BIT(MT6360_ADCEN_SHFT);
> + tmp[(chan->channel / 8) ? 0 : 1] |= BIT(chan->channel % 8);
Hmm. This a write into a 16 bit big endian buffer I think. Would it better
to just treat it as an __be16?
> + ret = regmap_bulk_write(mad->regmap,
> + MT6360_PMU_ADC_CONFIG, tmp, sizeof(tmp));
> + if (ret < 0)
> + goto err_adc_init;
> + start_t = ktime_get();
> + predict_end_t = ktime_add_ms(
> + mad->last_off_timestamps[chan->channel], 50);
> + if (ktime_after(start_t, predict_end_t))
> + predict_end_t = ktime_add_ms(start_t, 25);
> + else
> + predict_end_t = ktime_add_ms(start_t, 75);
> + enable_irq(mad->irq);
So why do we need to only enable the irq here. I would have assumed it
would not happen until we trigger a read?
> +retry:
> + reinit_completion(&mad->adc_complete);
Always reinit completion before enabling the irq. Too many races happen when doing
it the other way around.
> + /* wait for conversion to complete */
> + timeout = wait_for_completion_interruptible_timeout(
> + &mad->adc_complete, msecs_to_jiffies(200));
> + if (timeout == 0) {
> + ret = -ETIMEDOUT;
> + goto err_adc_conv;
> + } else if (timeout < 0) {
> + ret = -EINTR;
> + goto err_adc_conv;
> + }
> + memset(rpt, 0, sizeof(rpt));
If reading the whole size of rpt we should never need to zero it.
> + ret = regmap_bulk_read(mad->regmap,
> + MT6360_PMU_ADC_RPT_1, rpt, sizeof(rpt));
> + if (ret < 0)
> + goto err_adc_conv;
> + /* get report channel */
> + if ((rpt[0] & MT6360_RPTCH_MASK) != chan->channel) {
> + dev_dbg(&iio_dev->dev,
> + "not wanted channel report [%02x]\n", rpt[0]);
> + goto retry;
> + }
> + if (!ktime_after(ktime_get(), predict_end_t)) {
> + dev_dbg(&iio_dev->dev, "time is not after 26ms chan_time\n");
> + goto retry;
> + }
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + *val = (rpt[1] << 8) | rpt[2];
As mentioned below. It's normally an either / or for processed and raw.
When conversion is linear we prefer to push the maths to userspace.
If it's not you have no real choice but to do it in kernel and the raw
reading isn't much use.
> + break;
> + case IIO_CHAN_INFO_PROCESSED:
> + *val = (rpt[1] << 8) | rpt[2];
> + ret = mt6360_adc_get_process_val(mad, chan->channel, val);
> + if (ret < 0)
> + goto err_adc_conv;
> + break;
> + default:
> + break;
> + }
> + ret = IIO_VAL_INT;
> +err_adc_conv:
> + disable_irq(mad->irq);
> + /* whatever disable all channel and keep adc_en*/
> + memset(tmp, 0, sizeof(tmp));
> + tmp[0] |= BIT(MT6360_ADCEN_SHFT);
> + regmap_bulk_write(mad->regmap, MT6360_PMU_ADC_CONFIG, tmp, sizeof(tmp));
> + mad->last_off_timestamps[chan->channel] = ktime_get();
> + /* set prefer channel to 0xf */
> + regmap_update_bits(mad->regmap, MT6360_PMU_ADC_RPT_1,
> + MT6360_PREFERCH_MASK, 0xF << MT6360_PREFERCH_SHFT);
> +err_adc_init:
> + mutex_unlock(&mad->adc_lock);
> + return ret;
> +}
> +
> +static const struct iio_info mt6360_adc_iio_info = {
> + .read_raw = mt6360_adc_read_raw,
> +};
> +
> +#define MT6360_ADC_CHAN(_idx, _type) { \
> + .type = _type, \
> + .channel = MT6360_CHAN_##_idx, \
> + .scan_index = MT6360_CHAN_##_idx, \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 32, \
> + .storagebits = 32, \
> + .shift = 0, \
> + .endianness = IIO_CPU, \
> + }, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_PROCESSED), \
It very rarely makes sense to provide both raw and processed.
Why are you doing so here?
> + .datasheet_name = #_idx, \
> + .indexed = 1, \
> +}
> +
> +static const struct iio_chan_spec mt6360_adc_channels[] = {
> + MT6360_ADC_CHAN(USBID, IIO_VOLTAGE),
> + MT6360_ADC_CHAN(VBUSDIV5, IIO_VOLTAGE),
> + MT6360_ADC_CHAN(VBUSDIV2, IIO_VOLTAGE),
> + MT6360_ADC_CHAN(VSYS, IIO_VOLTAGE),
> + MT6360_ADC_CHAN(VBAT, IIO_VOLTAGE),
> + MT6360_ADC_CHAN(IBUS, IIO_CURRENT),
> + MT6360_ADC_CHAN(IBAT, IIO_CURRENT),
> + MT6360_ADC_CHAN(CHG_VDDP, IIO_VOLTAGE),
> + MT6360_ADC_CHAN(TEMP_JC, IIO_TEMP),
> + MT6360_ADC_CHAN(VREF_TS, IIO_VOLTAGE),
> + MT6360_ADC_CHAN(TS, IIO_VOLTAGE),
> + IIO_CHAN_SOFT_TIMESTAMP(MT6360_CHAN_MAX),
> +};
> +
> +static irqreturn_t mt6360_pmu_adc_donei_handler(int irq, void *data)
> +{
> + struct mt6360_adc_data *mad = iio_priv(data);
> +
> + complete(&mad->adc_complete);
> + return IRQ_HANDLED;
> +}
> +
> +static int mt6360_adc_scan_task_threadfn(void *data)
> +{
> + struct mt6360_adc_data *mad = data;
> + struct iio_dev *indio_dev = iio_priv_to_dev(mad);
> + int channel_vals[MT6360_CHAN_MAX];
__aligned(8) for the above as iio_push_to_buffers_with_timestamp
needs to be able to write an aligned 8 byte timestamp.
Also is that buffer long enough to allow for that timestamp?
}
> + int i, bit, var = 0;
> + int ret;
> +
> + while (!kthread_should_stop()) {
So this is spinning as fast as possible? Seems like some sort
of delay should be in here. Why not use an existing trigger
to do this given it's sample on demand?
We have both the hrtimer trigger and a tight loop trigger
to handle usecases like this?
Is it todo with in kernel consumers?
> + memset(channel_vals, 0, sizeof(channel_vals));
> + i = 0;
> + for_each_set_bit(bit, indio_dev->active_scan_mask,
> + indio_dev->masklength) {
> + ret = mt6360_adc_read_raw(indio_dev,
> + mt6360_adc_channels + bit,
> + &var, NULL,
> + IIO_CHAN_INFO_PROCESSED);
> + if (ret < 0)
> + dev_err(mad->dev, "get adc[%d] fail\n", bit);
> + channel_vals[i++] = var;
> + if (kthread_should_stop())
> + goto out;
> + }
> + iio_push_to_buffers_with_timestamp(indio_dev, channel_vals,
> + iio_get_time_ns(indio_dev));
> + }
> +out:
> + do_exit(0);
> + return 0;
> +}
> +
> +static int mt6360_adc_iio_post_enable(struct iio_dev *iio_dev)
> +{
> + struct mt6360_adc_data *mad = iio_priv(iio_dev);
> +
> + mad->scan_task = kthread_run(mt6360_adc_scan_task_threadfn, mad,
> + "scan_thread.%s", dev_name(&iio_dev->dev));
> + return PTR_ERR_OR_ZERO(mad->scan_task);
> +}
> +
> +static int mt6360_adc_iio_pre_disable(struct iio_dev *iio_dev)
> +{
> + struct mt6360_adc_data *mad = iio_priv(iio_dev);
> +
> + if (mad->scan_task) {
How could you get here without this being true?
> + kthread_stop(mad->scan_task);
> + mad->scan_task = NULL;
> + }
> + return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops mt6360_adc_iio_setup_ops = {
> + .postenable = mt6360_adc_iio_post_enable,
> + .predisable = mt6360_adc_iio_pre_disable,
> +};
> +
> +static int mt6360_adc_iio_device_register(struct iio_dev *indio_dev)
> +{
> + struct mt6360_adc_data *mad = iio_priv(indio_dev);
> + struct iio_buffer *buffer;
> + int ret;
> +
> + indio_dev->name = dev_name(mad->dev);
> + indio_dev->dev.parent = mad->dev;
> + indio_dev->dev.of_node = mad->dev->of_node;
> + indio_dev->info = &mt6360_adc_iio_info;
> + indio_dev->channels = mt6360_adc_channels;
> + indio_dev->num_channels = ARRAY_SIZE(mt6360_adc_channels);
> + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> + indio_dev->setup_ops = &mt6360_adc_iio_setup_ops;
> + buffer = devm_iio_kfifo_allocate(mad->dev);
> + if (!buffer)
> + return -ENOMEM;
> + iio_device_attach_buffer(indio_dev, buffer);
> + ret = devm_iio_device_register(mad->dev, indio_dev);
> + if (ret < 0) {
Where possible use simple if (ret) as is can slightly simplify flow.
> + dev_err(mad->dev, "Failed to register iio device\n");
> + return ret;
> + }
> + return 0;
return ret; drop it out of the above brackets having changed
the check.
> +}
> +
> +static inline int mt6360_adc_reset(struct mt6360_adc_data *info)
> +{
> + u8 tmp[3] = {0x80, 0, 0};
> + ktime_t all_off_time;
> + int i;
> +
> + all_off_time = ktime_get();
> + for (i = 0; i < MT6360_CHAN_MAX; i++)
> + info->last_off_timestamps[i] = all_off_time;
> + /* enable adc_en, clear adc_chn_en/zcv/en/adc_wait_t/adc_idle_t */
> + return regmap_bulk_write(info->regmap,
> + MT6360_PMU_ADC_CONFIG, tmp, sizeof(tmp));
> +}
> +
> +static int mt6360_adc_probe(struct platform_device *pdev)
> +{
> + struct mt6360_adc_data *mad;
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*mad));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + mad = iio_priv(indio_dev);
> + mad->dev = &pdev->dev;
> + init_completion(&mad->adc_complete);
> + mutex_init(&mad->adc_lock);
> + platform_set_drvdata(pdev, indio_dev);
> +
> + mad->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> + if (!mad->regmap) {
> + dev_err(&pdev->dev, "Failed to get parent regmap\n");
> + return -ENODEV;
> + }
> +
> + ret = mt6360_adc_reset(mad);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "Failed to reset adc\n");
> + return ret;
> + }
> +
> + ret = mt6360_adc_iio_device_register(indio_dev);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "Failed to register iio device\n");
> + return ret;
> + }
> +
> + mad->irq = platform_get_irq_byname(pdev, "adc_donei");
> + if (mad->irq < 0) {
> + dev_err(&pdev->dev, "Failed to get adc_done irq\n");
> + return mad->irq;
> + }
> +
> + irq_set_status_flags(mad->irq, IRQ_NOAUTOEN);
> + ret = devm_request_threaded_irq(&pdev->dev, mad->irq, NULL,
> + mt6360_pmu_adc_donei_handler,
> + IRQF_TRIGGER_FALLING, "adc_donei",
> + platform_get_drvdata(pdev));
> + if (ret < 0) {
> + dev_err(&pdev->dev, "Failed to register adc_done irq\n");
> + return ret;
> + }
It's unusual to register an interrupt 'after' we have made the userspace
and inkernel ABIs available (as they can often cause the interrupt to fire).
So I'd normally expect the iio_device_register call to be very last one
in probe. Why is it not in this case?
> +
> + return 0;
> +}
> +
> +static int mt6360_adc_remove(struct platform_device *pdev)
> +{
> + struct mt6360_adc_data *mad = platform_get_drvdata(pdev);
> +
> + if (mad->scan_task)
> + kthread_stop(mad->scan_task);
I'm a bit surprised this is needed. Remove should result in
iio_device_unregister being called which should smoothly shut
down any buffered capture that is ongoing and I would have
assumed would hence stop the thread.
You may have an ordering issue though.
> +
> + return 0;
> +}
> +
> +static const struct of_device_id __maybe_unused mt6360_adc_of_id[] = {
> + { .compatible = "mediatek,mt6360_adc", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, mt6360_adc_of_id);
> +
> +static struct platform_driver mt6360_adc_driver = {
> + .driver = {
> + .name = "mt6360_adc",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(mt6360_adc_of_id),
Whilst it is fairly unlikely I guess that someone might want to use
ACPI to probe this lets not prevent it without good reason
(via the magic PRP0001) ID so please drop the of_match_ptr protection.
> + },
> + .probe = mt6360_adc_probe,
> + .remove = mt6360_adc_remove,
> +};
> +module_platform_driver(mt6360_adc_driver);
> +
> +MODULE_AUTHOR("Gene Chen <gene_chen@richtek.com>");
> +MODULE_DESCRIPTION("MT6360 ADC Driver");
> +MODULE_LICENSE("GPL v2");
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 004/105] clk: bcm: Add BCM2711 DVP driver
From: Nicolas Saenz Julienne @ 2020-06-04 17:26 UTC (permalink / raw)
To: Maxime Ripard, Eric Anholt
Cc: devicetree, Tim Gover, Dave Stevenson, Stephen Boyd,
Michael Turquette, linux-kernel, dri-devel, linux-clk,
Rob Herring, bcm-kernel-feedback-list, linux-rpi-kernel,
Phil Elwell, linux-arm-kernel
In-Reply-To: <6615a61b8af240e3d10f8890e4b2462ccdaac9b9.1590594512.git-series.maxime@cerno.tech>
[-- Attachment #1.1: Type: text/plain, Size: 6101 bytes --]
Hi Maxime,
On Wed, 2020-05-27 at 17:47 +0200, Maxime Ripard wrote:
> The HDMI block has a block that controls clocks and reset signals to the
> HDMI0 and HDMI1 controllers.
Why not having two separate drivers?
> Let's expose that through a clock driver implementing a clock and reset
> provider.
>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
> drivers/clk/bcm/Kconfig | 11 +++-
> drivers/clk/bcm/Makefile | 1 +-
> drivers/clk/bcm/clk-bcm2711-dvp.c | 127 +++++++++++++++++++++++++++++++-
> 3 files changed, 139 insertions(+)
> create mode 100644 drivers/clk/bcm/clk-bcm2711-dvp.c
>
> diff --git a/drivers/clk/bcm/Kconfig b/drivers/clk/bcm/Kconfig
> index 8c83977a7dc4..784f12c72365 100644
> --- a/drivers/clk/bcm/Kconfig
> +++ b/drivers/clk/bcm/Kconfig
> @@ -1,4 +1,15 @@
> # SPDX-License-Identifier: GPL-2.0-only
> +
> +config CLK_BCM2711_DVP
> + tristate "Broadcom BCM2711 DVP support"
> + depends on ARCH_BCM2835 ||COMPILE_TEST
> + depends on COMMON_CLK
> + default ARCH_BCM2835
> + select RESET_SIMPLE
> + help
> + Enable common clock framework support for the Broadcom BCM2711
> + DVP Controller.
> +
> config CLK_BCM2835
> bool "Broadcom BCM2835 clock support"
> depends on ARCH_BCM2835 || ARCH_BRCMSTB || COMPILE_TEST
> diff --git a/drivers/clk/bcm/Makefile b/drivers/clk/bcm/Makefile
> index 0070ddf6cdd2..2c1349062147 100644
> --- a/drivers/clk/bcm/Makefile
> +++ b/drivers/clk/bcm/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_CLK_BCM_KONA) += clk-kona-setup.o
> obj-$(CONFIG_CLK_BCM_KONA) += clk-bcm281xx.o
> obj-$(CONFIG_CLK_BCM_KONA) += clk-bcm21664.o
> obj-$(CONFIG_COMMON_CLK_IPROC) += clk-iproc-armpll.o clk-iproc-pll.o
> clk-iproc-asiu.o
> +obj-$(CONFIG_CLK_BCM2835) += clk-bcm2711-dvp.o
> obj-$(CONFIG_CLK_BCM2835) += clk-bcm2835.o
> obj-$(CONFIG_CLK_BCM2835) += clk-bcm2835-aux.o
> obj-$(CONFIG_CLK_RASPBERRYPI) += clk-raspberrypi.o
> diff --git a/drivers/clk/bcm/clk-bcm2711-dvp.c b/drivers/clk/bcm/clk-bcm2711-
> dvp.c
> new file mode 100644
> index 000000000000..c1c4b5857d32
> --- /dev/null
> +++ b/drivers/clk/bcm/clk-bcm2711-dvp.c
> @@ -0,0 +1,127 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// Copyright 2020 Cerno
> +
> +#include <linux/clk-provider.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/reset/reset-simple.h>
> +
> +#define DVP_HT_RPI_SW_INIT 0x04
> +#define DVP_HT_RPI_MISC_CONFIG 0x08
> +
> +#define NR_CLOCKS 2
> +#define NR_RESETS 6
> +
> +struct clk_dvp {
> + struct clk_hw_onecell_data *data;
> + struct reset_simple_data reset;
> +};
> +
> +static const struct clk_parent_data clk_dvp_parent = {
> + .index = 0,
> +};
> +
> +static int clk_dvp_probe(struct platform_device *pdev)
> +{
> + struct clk_hw_onecell_data *data;
> + struct resource *res;
> + struct clk_dvp *dvp;
> + void __iomem *base;
> + int ret;
> +
> + dvp = devm_kzalloc(&pdev->dev, sizeof(*dvp), GFP_KERNEL);
> + if (!dvp)
> + return -ENOMEM;
> + platform_set_drvdata(pdev, dvp);
> +
> + dvp->data = devm_kzalloc(&pdev->dev,
> + struct_size(dvp->data, hws, NR_CLOCKS),
> + GFP_KERNEL);
> + if (!dvp->data)
> + return -ENOMEM;
> + data = dvp->data;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + base = devm_ioremap_resource(&pdev->dev, res);
I think the cool function to use these days is
devm_platform_get_and_ioremap_resource().
Regards,
Nicolas
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + dvp->reset.rcdev.owner = THIS_MODULE;
> + dvp->reset.rcdev.nr_resets = NR_RESETS;
> + dvp->reset.rcdev.ops = &reset_simple_ops;
> + dvp->reset.rcdev.of_node = pdev->dev.of_node;
> + dvp->reset.membase = base + DVP_HT_RPI_SW_INIT;
> + spin_lock_init(&dvp->reset.lock);
> +
> + ret = reset_controller_register(&dvp->reset.rcdev);
> + if (ret)
> + return ret;
> +
> + data->hws[0] = clk_hw_register_gate_parent_data(&pdev->dev,
> + "hdmi0-108MHz",
> + &clk_dvp_parent, 0,
> + base +
> DVP_HT_RPI_MISC_CONFIG, 3,
> + CLK_GATE_SET_TO_DISABLE,
> + &dvp->reset.lock);
> + if (IS_ERR(data->hws[0])) {
> + ret = PTR_ERR(data->hws[0]);
> + goto unregister_reset;
> + }
> +
> + data->hws[1] = clk_hw_register_gate_parent_data(&pdev->dev,
> + "hdmi1-108MHz",
> + &clk_dvp_parent, 0,
> + base +
> DVP_HT_RPI_MISC_CONFIG, 4,
> + CLK_GATE_SET_TO_DISABLE,
> + &dvp->reset.lock);
> + if (IS_ERR(data->hws[1])) {
> + ret = PTR_ERR(data->hws[1]);
> + goto unregister_clk0;
> + }
> +
> + data->num = NR_CLOCKS;
> + ret = of_clk_add_hw_provider(pdev->dev.of_node, of_clk_hw_onecell_get,
> + data);
> + if (ret)
> + goto unregister_clk1;
> +
> + return 0;
> +
> +unregister_clk1:
> + clk_hw_unregister_gate(data->hws[1]);
> +
> +unregister_clk0:
> + clk_hw_unregister_gate(data->hws[0]);
> +
> +unregister_reset:
> + reset_controller_unregister(&dvp->reset.rcdev);
> + return ret;
> +};
> +
> +static int clk_dvp_remove(struct platform_device *pdev)
> +{
> + struct clk_dvp *dvp = platform_get_drvdata(pdev);
> + struct clk_hw_onecell_data *data = dvp->data;
> +
> + clk_hw_unregister_gate(data->hws[1]);
> + clk_hw_unregister_gate(data->hws[0]);
> + reset_controller_unregister(&dvp->reset.rcdev);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id clk_dvp_dt_ids[] = {
> + { .compatible = "brcm,brcm2711-dvp", },
> + { /* sentinel */ }
> +};
> +
> +static struct platform_driver clk_dvp_driver = {
> + .probe = clk_dvp_probe,
> + .remove = clk_dvp_remove,
> + .driver = {
> + .name = "brcm2711-dvp",
> + .of_match_table = clk_dvp_dt_ids,
> + },
> +};
> +module_platform_driver(clk_dvp_driver);
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] arm64: debug: mark a function as __init to save some memory
From: Will Deacon @ 2020-06-04 17:25 UTC (permalink / raw)
To: catalin.marinas, mhiramat, Christophe JAILLET, gregkh, paulmck,
tglx, dianders
Cc: kernel-janitors, Will Deacon, linux-kernel, linux-arm-kernel
In-Reply-To: <20200531110015.598607-1-christophe.jaillet@wanadoo.fr>
On Sun, 31 May 2020 13:00:15 +0200, Christophe JAILLET wrote:
> 'debug_monitors_init()' is only called via 'postcore_initcall'.
> It can be marked as __init to save a few bytes of memory.
Applied to arm64 (for-next/core), thanks!
[1/1] arm64: debug: mark a function as __init to save some memory
https://git.kernel.org/arm64/c/5311ebfb612f
Cheers,
--
Will
https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] ASoC: meson: fix memory leak of links if allocation of ldata fails
From: Colin King @ 2020-06-04 17:12 UTC (permalink / raw)
To: Jerome Brunet, Liam Girdwood, Mark Brown, Jaroslav Kysela,
Takashi Iwai, Kevin Hilman, alsa-devel, linux-arm-kernel,
linux-amlogic
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Currently if the allocation of ldata fails the error return path
does not kfree the allocated links object. Fix this by adding
an error exit return path that performs the necessary kfree'ing.
Addresses-Coverity: ("Resource leak")
Fixes: 7864a79f37b5 ("ASoC: meson: add axg sound card support")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
sound/soc/meson/meson-card-utils.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/sound/soc/meson/meson-card-utils.c b/sound/soc/meson/meson-card-utils.c
index 2ca8c98e204f..5a4a91c88734 100644
--- a/sound/soc/meson/meson-card-utils.c
+++ b/sound/soc/meson/meson-card-utils.c
@@ -49,19 +49,26 @@ int meson_card_reallocate_links(struct snd_soc_card *card,
links = krealloc(priv->card.dai_link,
num_links * sizeof(*priv->card.dai_link),
GFP_KERNEL | __GFP_ZERO);
+ if (!links)
+ goto err_links;
+
ldata = krealloc(priv->link_data,
num_links * sizeof(*priv->link_data),
GFP_KERNEL | __GFP_ZERO);
-
- if (!links || !ldata) {
- dev_err(priv->card.dev, "failed to allocate links\n");
- return -ENOMEM;
- }
+ if (!ldata)
+ goto err_ldata;
priv->card.dai_link = links;
priv->link_data = ldata;
priv->card.num_links = num_links;
return 0;
+
+err_ldata:
+ kfree(links);
+err_links:
+ dev_err(priv->card.dev, "failed to allocate links\n");
+ return -ENOMEM;
+
}
EXPORT_SYMBOL_GPL(meson_card_reallocate_links);
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v5 0/2] Small devm helper for devm implementations
From: Greg Kroah-Hartman @ 2020-06-04 17:10 UTC (permalink / raw)
To: Marc Gonzalez
Cc: Mark Rutland, linux-clk, LKML, Arnd Bergmann, Kuninori Morimoto,
Ard Biesheuvel, Stephen Boyd, Suzuki Poulose, Michael Turquette,
Dmitry Torokhov, Rafael Wysocki, Russell King, Bjorn Andersson,
Geert Uytterhoeven, Linux ARM, Robin Murphy, Sudip Mukherjee,
Guenter Roeck
In-Reply-To: <217b892d-678f-8c32-b9ab-a3dd238c197a@free.fr>
On Thu, Jun 04, 2020 at 06:13:21PM +0200, Marc Gonzalez wrote:
> Looks like this series has fallen through the cracks :(
> Greg, you would be taking the drivers/base/devres.c changes?
> As mentioned in patch 2, "This patch needs testing on a platform with many clocks."
> (I've only tested using a trivial kernel module)
I can't do anything during the merge window, sorry. Please feel free to
resend it after 5.8-rc1 is out and I will be glad to review it then.
thanks,
greg k-h
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v8 0/5] support reserving crashkernel above 4G on arm64 kdump
From: Nicolas Saenz Julienne @ 2020-06-04 17:01 UTC (permalink / raw)
To: Bhupesh Sharma, John Donnelly
Cc: Devicetree List, Arnd Bergmann, Baoquan He,
Linux Doc Mailing List, chenzhou, Catalin Marinas, RuiRui Yang,
Prabhakar Kushwaha, kexec mailing list, Linux Kernel Mailing List,
Rob Herring, Simon Horman, James Morse, guohanjun,
Thomas Gleixner, Prabhakar Kushwaha, Will Deacon, Ingo Molnar,
linux-arm-kernel
In-Reply-To: <CACi5LpN-+NRnaDoWWWidbzma8BNzmofA5FQBV=cPF1Mc84FpFg@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 16696 bytes --]
On Thu, 2020-06-04 at 01:17 +0530, Bhupesh Sharma wrote:
> Hi All,
>
> On Wed, Jun 3, 2020 at 9:03 PM John Donnelly <john.p.donnelly@oracle.com>
> wrote:
> >
> >
> > > On Jun 3, 2020, at 8:20 AM, chenzhou <chenzhou10@huawei.com> wrote:
> > >
> > > Hi,
> > >
> > >
> > > On 2020/6/3 19:47, Prabhakar Kushwaha wrote:
> > > > Hi Chen,
> > > >
> > > > On Tue, Jun 2, 2020 at 8:12 PM John Donnelly <john.p.donnelly@oracle.com
> > > > > wrote:
> > > > >
> > > > > > On Jun 2, 2020, at 12:38 AM, Prabhakar Kushwaha <
> > > > > > prabhakar.pkin@gmail.com> wrote:
> > > > > >
> > > > > > On Tue, Jun 2, 2020 at 3:29 AM John Donnelly <
> > > > > > john.p.donnelly@oracle.com> wrote:
> > > > > > > Hi . See below !
> > > > > > >
> > > > > > > > On Jun 1, 2020, at 4:02 PM, Bhupesh Sharma <bhsharma@redhat.com>
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > Hi John,
> > > > > > > >
> > > > > > > > On Tue, Jun 2, 2020 at 1:01 AM John Donnelly <
> > > > > > > > John.P.donnelly@oracle.com> wrote:
> > > > > > > > > Hi,
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On 6/1/20 7:02 AM, Prabhakar Kushwaha wrote:
> > > > > > > > > > Hi Chen,
> > > > > > > > > >
> > > > > > > > > > On Thu, May 21, 2020 at 3:05 PM Chen Zhou <
> > > > > > > > > > chenzhou10@huawei.com> wrote:
> > > > > > > > > > > This patch series enable reserving crashkernel above 4G in
> > > > > > > > > > > arm64.
> > > > > > > > > > >
> > > > > > > > > > > There are following issues in arm64 kdump:
> > > > > > > > > > > 1. We use crashkernel=X to reserve crashkernel below 4G,
> > > > > > > > > > > which will fail
> > > > > > > > > > > when there is no enough low memory.
> > > > > > > > > > > 2. Currently, crashkernel=Y@X can be used to reserve
> > > > > > > > > > > crashkernel above 4G,
> > > > > > > > > > > in this case, if swiotlb or DMA buffers are required,
> > > > > > > > > > > crash dump kernel
> > > > > > > > > > > will boot failure because there is no low memory available
> > > > > > > > > > > for allocation.
> > > > > > > > > > >
> > > > > > > > > > We are getting "warn_alloc" [1] warning during boot of kdump
> > > > > > > > > > kernel
> > > > > > > > > > with bootargs as [2] of primary kernel.
> > > > > > > > > > This error observed on ThunderX2 ARM64 platform.
> > > > > > > > > >
> > > > > > > > > > It is observed with latest upstream tag (v5.7-rc3) with this
> > > > > > > > > > patch set
> > > > > > > > > > and
> > > > > > > > > >
https://urldefense.com/v3/__https://lists.infradead.org/pipermail/kexec/2020-May/025128.html__;!!GqivPVa7Brio!LnTSARkCt0V0FozR0KmqooaH5ADtdXvs3mPdP3KRVqALmvSK2VmCkIPIhsaxbiIAAlzu$
> > > > > > > > > > Also **without** this patch-set
> > > > > > > > > > "
> > > > > > > > > >
https://urldefense.com/v3/__https://www.spinics.net/lists/arm-kernel/msg806882.html__;!!GqivPVa7Brio!LnTSARkCt0V0FozR0KmqooaH5ADtdXvs3mPdP3KRVqALmvSK2VmCkIPIhsaxbjC6ujMA$
> > > > > > > > > > "
> > > > > > > > > >
> > > > > > > > > > This issue comes whenever crashkernel memory is reserved
> > > > > > > > > > after 0xc000_0000.
> > > > > > > > > > More details discussed earlier in
> > > > > > > > > >
https://urldefense.com/v3/__https://www.spinics.net/lists/arm-kernel/msg806882.html__;!!GqivPVa7Brio!LnTSARkCt0V0FozR0KmqooaH5ADtdXvs3mPdP3KRVqALmvSK2VmCkIPIhsaxbjC6ujMA$
without
> > > > > > > > > > any
> > > > > > > > > > solution
> > > > > > > > > >
> > > > > > > > > > This patch-set is expected to solve similar kind of issue.
> > > > > > > > > > i.e. low memory is only targeted for DMA, swiotlb; So above
> > > > > > > > > > mentioned
> > > > > > > > > > observation should be considered/fixed. .
> > > > > > > > > >
> > > > > > > > > > --pk
> > > > > > > > > >
> > > > > > > > > > [1]
> > > > > > > > > > [ 30.366695] DMI: Cavium Inc. Saber/Saber, BIOS
> > > > > > > > > > TX2-FW-Release-3.1-build_01-2803-g74253a541a mm/dd/yyyy
> > > > > > > > > > [ 30.367696] NET: Registered protocol family 16
> > > > > > > > > > [ 30.369973] swapper/0: page allocation failure: order:6,
> > > > > > > > > > mode:0x1(GFP_DMA), nodemask=(null),cpuset=/,mems_allowed=0
> > > > > > > > > > [ 30.369980] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
> > > > > > > > > > 5.7.0-rc3+ #121
> > > > > > > > > > [ 30.369981] Hardware name: Cavium Inc. Saber/Saber, BIOS
> > > > > > > > > > TX2-FW-Release-3.1-build_01-2803-g74253a541a mm/dd/yyyy
> > > > > > > > > > [ 30.369984] Call trace:
> > > > > > > > > > [ 30.369989] dump_backtrace+0x0/0x1f8
> > > > > > > > > > [ 30.369991] show_stack+0x20/0x30
> > > > > > > > > > [ 30.369997] dump_stack+0xc0/0x10c
> > > > > > > > > > [ 30.370001] warn_alloc+0x10c/0x178
> > > > > > > > > > [ 30.370004] __alloc_pages_slowpath.constprop.111+0xb10/0
> > > > > > > > > > xb50
> > > > > > > > > > [ 30.370006] __alloc_pages_nodemask+0x2b4/0x300
> > > > > > > > > > [ 30.370008] alloc_page_interleave+0x24/0x98
> > > > > > > > > > [ 30.370011] alloc_pages_current+0xe4/0x108
> > > > > > > > > > [ 30.370017] dma_atomic_pool_init+0x44/0x1a4
> > > > > > > > > > [ 30.370020] do_one_initcall+0x54/0x228
> > > > > > > > > > [ 30.370027] kernel_init_freeable+0x228/0x2cc
> > > > > > > > > > [ 30.370031] kernel_init+0x1c/0x110
> > > > > > > > > > [ 30.370034] ret_from_fork+0x10/0x18
> > > > > > > > > > [ 30.370036] Mem-Info:
> > > > > > > > > > [ 30.370064] active_anon:0 inactive_anon:0 isolated_anon:0
> > > > > > > > > > [ 30.370064] active_file:0 inactive_file:0
> > > > > > > > > > isolated_file:0
> > > > > > > > > > [ 30.370064] unevictable:0 dirty:0 writeback:0 unstable:0
> > > > > > > > > > [ 30.370064] slab_reclaimable:34 slab_unreclaimable:4438
> > > > > > > > > > [ 30.370064] mapped:0 shmem:0 pagetables:14 bounce:0
> > > > > > > > > > [ 30.370064] free:1537719 free_pcp:219 free_cma:0
> > > > > > > > > > [ 30.370070] Node 0 active_anon:0kB inactive_anon:0kB
> > > > > > > > > > active_file:0kB inactive_file:0kB unevictable:0kB
> > > > > > > > > > isolated(anon):0kB
> > > > > > > > > > isolated(file):0kB mapped:0kB dirty:0kB writeback:0kB
> > > > > > > > > > shmem:0kB
> > > > > > > > > > shmem_thp: 0kB shmem_pmdmapped: 0kB anon_thp: 0kB
> > > > > > > > > > writeback_tmp:0kB
> > > > > > > > > > unstable:0kB all_unreclaimable? no
> > > > > > > > > > [ 30.370073] Node 1 active_anon:0kB inactive_anon:0kB
> > > > > > > > > > active_file:0kB inactive_file:0kB unevictable:0kB
> > > > > > > > > > isolated(anon):0kB
> > > > > > > > > > isolated(file):0kB mapped:0kB dirty:0kB writeback:0kB
> > > > > > > > > > shmem:0kB
> > > > > > > > > > shmem_thp: 0kB shmem_pmdmapped: 0kB anon_thp: 0kB
> > > > > > > > > > writeback_tmp:0kB
> > > > > > > > > > unstable:0kB all_unreclaimable? no
> > > > > > > > > > [ 30.370079] Node 0 DMA free:0kB min:0kB low:0kB high:0kB
> > > > > > > > > > reserved_highatomic:0KB active_anon:0kB inactive_anon:0kB
> > > > > > > > > > active_file:0kB inactive_file:0kB unevictable:0kB
> > > > > > > > > > writepending:0kB
> > > > > > > > > > present:128kB managed:0kB mlocked:0kB kernel_stack:0kB
> > > > > > > > > > pagetables:0kB
> > > > > > > > > > bounce:0kB free_pcp:0kB local_pcp:0kB free_cma:0kB
> > > > > > > > > > [ 30.370084] lowmem_reserve[]: 0 250 6063 6063
> > > > > > > > > > [ 30.370090] Node 0 DMA32 free:256000kB min:408kB
> > > > > > > > > > low:664kB
> > > > > > > > > > high:920kB reserved_highatomic:0KB active_anon:0kB
> > > > > > > > > > inactive_anon:0kB
> > > > > > > > > > active_file:0kB inactive_file:0kB unevictable:0kB
> > > > > > > > > > writepending:0kB
> > > > > > > > > > present:269700kB managed:256000kB mlocked:0kB
> > > > > > > > > > kernel_stack:0kB
> > > > > > > > > > pagetables:0kB bounce:0kB free_pcp:0kB local_pcp:0kB
> > > > > > > > > > free_cma:0kB
> > > > > > > > > > [ 30.370094] lowmem_reserve[]: 0 0 5813 5813
> > > > > > > > > > [ 30.370100] Node 0 Normal free:5894876kB min:9552kB
> > > > > > > > > > low:15504kB
> > > > > > > > > > high:21456kB reserved_highatomic:0KB active_anon:0kB
> > > > > > > > > > inactive_anon:0kB
> > > > > > > > > > active_file:0kB inactive_file:0kB unevictable:0kB
> > > > > > > > > > writepending:0kB
> > > > > > > > > > present:8388608kB managed:5953112kB mlocked:0kB
> > > > > > > > > > kernel_stack:21672kB
> > > > > > > > > > pagetables:56kB bounce:0kB free_pcp:876kB local_pcp:176kB
> > > > > > > > > > free_cma:0kB
> > > > > > > > > > [ 30.370104] lowmem_reserve[]: 0 0 0 0
> > > > > > > > > > [ 30.370107] Node 0 DMA: 0*4kB 0*8kB 0*16kB 0*32kB 0*64kB
> > > > > > > > > > 0*128kB
> > > > > > > > > > 0*256kB 0*512kB 0*1024kB 0*2048kB 0*4096kB = 0kB
> > > > > > > > > > [ 30.370113] Node 0 DMA32: 0*4kB 0*8kB 0*16kB 0*32kB
> > > > > > > > > > 0*64kB 0*128kB
> > > > > > > > > > 0*256kB 0*512kB 0*1024kB 1*2048kB (M) 62*4096kB (M) =
> > > > > > > > > > 256000kB
> > > > > > > > > > [ 30.370119] Node 0 Normal: 2*4kB (M) 3*8kB (ME) 2*16kB
> > > > > > > > > > (UE) 3*32kB
> > > > > > > > > > (UM) 1*64kB (U) 2*128kB (M) 2*256kB (ME) 3*512kB (ME)
> > > > > > > > > > 3*1024kB (ME)
> > > > > > > > > > 3*2048kB (UME) 1436*4096kB (M) = 5893600kB
> > > > > > > > > > [ 30.370129] Node 0 hugepages_total=0 hugepages_free=0
> > > > > > > > > > hugepages_surp=0 hugepages_size=1048576kB
> > > > > > > > > > [ 30.370130] 0 total pagecache pages
> > > > > > > > > > [ 30.370132] 0 pages in swap cache
> > > > > > > > > > [ 30.370134] Swap cache stats: add 0, delete 0, find 0/0
> > > > > > > > > > [ 30.370135] Free swap = 0kB
> > > > > > > > > > [ 30.370136] Total swap = 0kB
> > > > > > > > > > [ 30.370137] 2164609 pages RAM
> > > > > > > > > > [ 30.370139] 0 pages HighMem/MovableOnly
> > > > > > > > > > [ 30.370140] 612331 pages reserved
> > > > > > > > > > [ 30.370141] 0 pages hwpoisoned
> > > > > > > > > > [ 30.370143] DMA: failed to allocate 256 KiB pool for
> > > > > > > > > > atomic
> > > > > > > > > > coherent allocation
> > > > > > > > >
> > > > > > > > > During my testing I saw the same error and Chen's solution
> > > > > > > > > corrected it .
> > > > > > > > Which combination you are using on your side? I am using
> > > > > > > > Prabhakar's
> > > > > > > > suggested environment and can reproduce the issue
> > > > > > > > with or without Chen's crashkernel support above 4G patchset.
> > > > > > > >
> > > > > > > > I am also using a ThunderX2 platform with latest makedumpfile
> > > > > > > > code and
> > > > > > > > kexec-tools (with the suggested patch
> > > > > > > > <
> > > > > > > >
https://urldefense.com/v3/__https://lists.infradead.org/pipermail/kexec/2020-May/025128.html__;!!GqivPVa7Brio!J6lUig58-Gw6TKZnEEYzEeSU36T-1SqlB1kImU00xtX_lss5Tx-JbUmLE9TJC3foXBLg$
> > > > > > > > >).
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Bhupesh
> > > > > > >
> > > > > > > I did this activity 5 months ago and I have moved on to other
> > > > > > > activities. My DMA failures were related to PCI devices that could
> > > > > > > not be enumerated because low-DMA space was not available when
> > > > > > > crashkernel was moved above 4G; I don’t recall the exact platform.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > For this failure ,
> > > > > > >
> > > > > > > > > > DMA: failed to allocate 256 KiB pool for atomic
> > > > > > > > > > coherent allocation
> > > > > > >
> > > > > > > Is due to :
> > > > > > >
> > > > > > >
> > > > > > > 3618082c
> > > > > > > ("arm64 use both ZONE_DMA and ZONE_DMA32")
> > > > > > >
> > > > > > > With the introduction of ZONE_DMA to support the Raspberry DMA
> > > > > > > region below 1G, the crashkernel is placed in the upper 4G
> > > > > > > ZONE_DMA_32 region. Since the crashkernel does not have access
> > > > > > > to the ZONE_DMA region, it prints out call trace during bootup.
> > > > > > >
> > > > > > > It is due to having this CONFIG item ON :
> > > > > > >
> > > > > > >
> > > > > > > CONFIG_ZONE_DMA=y
> > > > > > >
> > > > > > > Turning off ZONE_DMA fixes a issue and Raspberry PI 4 will
> > > > > > > use the device tree to specify memory below 1G.
> > > > > > >
> > > > > > >
> > > > > > Disabling ZONE_DMA is temporary solution. We may need proper
> > > > > > solution
> > > > >
> > > > > Perhaps the Raspberry platform configuration dependencies need
> > > > > separated from “server class” Arm equipment ? Or auto-configured on
> > > > > boot ? Consult an expert ;-)
> > > > >
> > > > >
> > > > >
> > > > > > > I would like to see Chen’s feature added , perhaps as
> > > > > > > EXPERIMENTAL, so we can get some configuration testing done on
> > > > > > > it. It corrects having a DMA zone in low memory while crash-
> > > > > > > kernel is above 4GB. This has been going on for a year now.
> > > > > > I will also like this patch to be added in Linux as early as
> > > > > > possible.
> > > > > >
> > > > > > Issue mentioned by me happens with or without this patch.
> > > > > >
> > > > > > This patch-set can consider fixing because it uses low memory for
> > > > > > DMA
> > > > > > & swiotlb only.
> > > > > > We can consider restricting crashkernel within the required range
> > > > > > like below
> > > > > >
> > > > > > diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> > > > > > index 7f9e5a6dc48c..bd67b90d35bd 100644
> > > > > > --- a/kernel/crash_core.c
> > > > > > +++ b/kernel/crash_core.c
> > > > > > @@ -354,7 +354,7 @@ int __init reserve_crashkernel_low(void)
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > - low_base = memblock_find_in_range(0, 1ULL << 32, low_size,
> > > > > > CRASH_ALIGN);
> > > > > > + low_base = memblock_find_in_range(0,0xc0000000, low_size,
> > > > > > CRASH_ALIGN);
> > > > > > if (!low_base) {
> > > > > > pr_err("Cannot reserve %ldMB crashkernel low memory,
> > > > > > please try smaller size.\n",
> > > > > > (unsigned long)(low_size >> 20));
> > > > > >
> > > > > >
> > > > > I suspect 0xc0000000 would need to be a CONFIG item and not
> > > > > hard-coded.
> > > > >
> > > > if you consider this as valid change, can you please incorporate as
> > > > part of your patch-set.
> > >
> > > After commit 1a8e1cef7 ("arm64: use both ZONE_DMA and ZONE_DMA32")，the 0-
> > > 4G memory is splited
> > > to DMA [mem 0x0000000000000000-0x000000003fffffff] and DMA32 [mem
> > > 0x0000000040000000-0x00000000ffffffff] on arm64.
> > >
> > > From the above discussion, on your platform, the low crashkernel fall in
> > > DMA32 region, but your environment needs to access DMA
> > > region, so there is the call trace.
> > >
> > > I have a question, why do you choose 0xc0000000 here?
> > >
> > > Besides, this is common code, we also need to consider about x86.
> > >
> >
> > + nsaenzjulienne@suse.de
Thanks for adding me to the conversation, and sorry for the headaches.
> >
> > Exactly . This is why it needs to be a CONFIG option for Raspberry
> > .., or device tree option.
> >
> >
> > We could revert 1a8e1cef7 since it broke Arm kdump too.
>
> Well, unfortunately the patch for commit 1a8e1cef7603 ("arm64: use
> both ZONE_DMA and ZONE_DMA32") was not Cc'ed to the kexec mailing
> list, thus we couldn't get many eyes on it for a thorough review from
> kexec/kdump p-o-v.
>
> Also we historically never had distinction in common arch code on the
> basis of the intended end use-case: embedded, server or automotive, so
> I am not sure introducing a Raspberry specific CONFIG option would be
> a good idea.
+1
From the distros perspective it's very important to keep a single kernel image.
> So, rather than reverting the patch, we can look at addressing the
> same properly this time - especially from a kdump p-o-v.
> This issue has been reported by some Red Hat arm64 partners with
> upstream kernel also and as we have noticed in the past as well,
> hardcoding the placement of the crashkernel base address (unless the
> base address is specified by a crashkernel=X@Y like bootargs) is also
> not a portable suggestion.
>
> I am working on a possible fix and will have more updates on the same
> in a day-or-two.
Please keep me in the loop, we've also had issues pointing to this reported by
SUSE partners. I can do some testing both on the RPi4 and on big servers that
need huge crashkernel sizes.
Regards,
Nicolas
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 0/5 v2] KASan for ARM
From: Ard Biesheuvel @ 2020-06-04 17:01 UTC (permalink / raw)
To: Linus Walleij
Cc: Rob Herring, Florian Fainelli, Abbott Liu, Russell King,
Andrey Ryabinin, Linux ARM
In-Reply-To: <CAMj1kXErHqaQod5nQCypLvyWET-K2-CEKvcGMPYzgfb=mgKK0A@mail.gmail.com>
On Thu, 4 Jun 2020 at 14:10, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Thu, 4 Jun 2020 at 13:26, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Thu, 4 Jun 2020 at 11:24, Linus Walleij <linus.walleij@linaro.org> wrote:
> > >
> > > On Wed, Jun 3, 2020 at 10:45 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > > > On Mon, Jun 1, 2020 at 6:37 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> > > >
> > > > > This branch got me a bit further,
> > > >
> > > > Thanks, at least we get improvements. :)
> > > >
> > > > > but still failed to fully initialize
> > > > > (see attached kasan.log), on another platform with a slightly different
> > > > > memory map, I ended up getting a different error (kasan2.log).
> > > >
> > > > I have this error too on a Qualcomm board, it is what I report
> > > > in the cover letter, that if I load the kernel into 0x40200000
> > > > this happens but when I load it into 0x50000000 it does not
> > > > happen.
> > >
> > > So this is what happens to me, even after I try to de-instrument
> > > the DT parsing code (maybe I do it all wrong...)
> > > This is done with that patch:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-integrator.git/commit/?h=kasan-apq8060-test&id=1cd83357f3c35b037400f6ec2547eeff074c578c
> > >
> > > If I boot from physical memory at 0x40200000
> > > fastboot --base 40200000 --cmdline "console=ttyMSM0,115200,n8" boot zImage
> > >
> > > kasan: populating shadow for b7040000, b75c0000
> > > kasan: populating shadow for b8000000, bb000000
> > > kasan: populating shadow for b6e00000, b7000000
> > > kasan: Kernel address sanitizer initialized
> > > 8<--- cut here ---
> > > Unable to handle kernel paging request at virtual address c30050b0
> > > pgd = (ptrval)
> > > [c30050b0] *pgd=00000000c
> > > Internal error: Oops: 5 [#1] PREEMPT SMP ARM
> > > Modules linked in:c
> > > CPU: 0 PID: 0 Comm: swapper Not tainted 5.7.0-00011-g1cd83357f3c3 #34
> > > Hardware name: Generic DT based system
> > > PC is at fdt_check_header+0x0/0x168
> > > LR is at __unflatten_device_tree+0x6c/0x338
> > > pc : [<c08e6968>] lr : [<c0d698a8>] psr: 60000093
> > > sp : c1e03db8 ip : cffffee0 fp : fffff000
> > > r10: 00000000 r9 : c2646000 r8 : 00000000
> > > r7 : c30050b0 r6 : c192e9e4 r5 : c19492e8 r4 : c21d7448
> > > r3 : 00000000 r2 : c2646000 r1 : 00000000 r0 : c30050b0
> > > (...)
> > > [<c08e6968>] (fdt_check_header) from [<c192e9e4>]
> > > (early_init_dt_alloc_memory_arch+0x0/0x64)
> > > [<c192e9e4>] (early_init_dt_alloc_memory_arch) from [<c1930264>]
> > > (unflatten_device_tree+0x34/0x44)
> > > [<c1930264>] (unflatten_device_tree) from [<c1905794>] (setup_arch+0xac4/0xde8)
> > > [<c1905794>] (setup_arch) from [<c1900b98>] (start_kernel+0xd8/0x634)
> > > [<c1900b98>] (start_kernel) from [<00000000>] (0x0)
> > > Code: e3a00020 e12fff1e e3a0001c e12fff1e (e5901000)
> > > random: get_random_bytes called from print_oops_end_marker+0x38/0x50
> > > with crng_init=0
> > > ---[ end trace 0000000000000000 ]---
> > > Kernel panic - not syncing: Attempted to kill the idle task!
> > > ---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]---
> > >
> >
> > I don't think we ever check whether the ATAGS/DTB pointer points into
> > memory that is described to the kernel as unreserved lowmem. We simply
> > call phys_to_virt() on it [in setup_machine_fdt()], and assume that by
> > the time we call unflatten_device_tree(), the same virtual address
> > still points to the DT contents.
> >
OK, so this is definitely not the issue. I am seeing very similar
issues, but in different places (two examples below)
What is notable here is that in both cases, a sizable chunk of memory
has one stack frame's worth of shadow bytes right in the middle. I
spent some time trying to figure out what is going on here, but I am
not a KASAN expert, so I am struggling a bit. Are we sure that all the
shadow is mapped correctly without physical pages being mapped more
than once?
[ 0.000000] ==================================================================
[ 0.000000] BUG: KASAN: stack-out-of-bounds in
memblock_alloc_try_nid+0x108/0x144
[ 0.000000] Write of size 19104 at addr e95c2560 by task swapper/0
[ 0.000000]
[ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.7.0+ #60
[ 0.000000] Hardware name: Generic DT based system
[ 0.000000] [<c0317634>] (unwind_backtrace) from [<c030f338>]
(show_stack+0x10/0x14)
[ 0.000000] [<c030f338>] (show_stack) from [<c0905130>]
(dump_stack+0xc4/0xdc)
[ 0.000000] [<c0905130>] (dump_stack) from [<c053b238>]
(print_address_description.constprop.0+0x34/0x444)
[ 0.000000] [<c053b238>] (print_address_description.constprop.0)
from [<c053b844>] (kasan_report+0x158/0x174)
[ 0.000000] [<c053b844>] (kasan_report) from [<c053c24c>]
(check_memory_region+0x94/0x1a4)
[ 0.000000] [<c053c24c>] (check_memory_region) from [<c053a640>]
(memset+0x20/0x3c)
[ 0.000000] [<c053a640>] (memset) from [<c242fa78>]
(memblock_alloc_try_nid+0x108/0x144)
[ 0.000000] [<c242fa78>] (memblock_alloc_try_nid) from [<c24c857c>]
(early_init_dt_alloc_memory_arch+0x30/0x60)
[ 0.000000] [<c24c857c>] (early_init_dt_alloc_memory_arch) from
[<c128cd3c>] (__unflatten_device_tree+0x5c/0x11c)
[ 0.000000] [<c128cd3c>] (__unflatten_device_tree) from
[<c24c9c88>] (unflatten_device_tree+0x34/0x44)
[ 0.000000] [<c24c9c88>] (unflatten_device_tree) from [<c2405a4c>]
(setup_arch+0xc00/0x10f4)
[ 0.000000] [<c2405a4c>] (setup_arch) from [<c2400c2c>]
(start_kernel+0xd4/0x610)
[ 0.000000] [<c2400c2c>] (start_kernel) from [<00000000>] (0x0)
[ 0.000000]
[ 0.000000] The buggy address belongs to the page:
[ 0.000000] page:efd24840 refcount:1 mapcount:0 mapping:00000000 index:0x0
[ 0.000000] flags: 0x0()
[ 0.000000] raw: 00000000 efd24844 efd24844 00000000 00000000
00000000 ffffffff 00000001
[ 0.000000] page dumped because: kasan: bad access detected
[ 0.000000]
[ 0.000000] Memory state around the buggy address:
[ 0.000000] e95c3c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 0.000000] e95c3c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 0.000000] >e95c3d00: f1 f1 f1 f1 f1 f1 04 f2 04 f3 f3 f3 00 00 00 00
[ 0.000000] ^
[ 0.000000] e95c3d80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 0.000000] e95c3e00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 0.000000] ==================================================================
[ 50.235136] ==================================================================
[ 50.238107] BUG: KASAN: stack-out-of-bounds in blk_add_partitions+0x1e8/0x6f8
[ 50.239318] Write of size 32768 at addr f08c6000 by task swapper/0/1
[ 50.240226]
[ 50.241432] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.7.0+ #60
[ 50.242402] Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015
[ 50.244206] [<c0317634>] (unwind_backtrace) from [<c030f338>]
(show_stack+0x10/0x14)
[ 50.245566] [<c030f338>] (show_stack) from [<c0905130>]
(dump_stack+0xc4/0xdc)
[ 50.246823] [<c0905130>] (dump_stack) from [<c053b384>]
(print_address_description.constprop.0+0x180/0x444)
[ 50.248429] [<c053b384>] (print_address_description.constprop.0)
from [<c053b844>] (kasan_report+0x158/0x174)
[ 50.250113] [<c053b844>] (kasan_report) from [<c053c24c>]
(check_memory_region+0x94/0x1a4)
[ 50.251495] [<c053c24c>] (check_memory_region) from [<c053a640>]
(memset+0x20/0x3c)
[ 50.252787] [<c053a640>] (memset) from [<c0868954>]
(blk_add_partitions+0x1e8/0x6f8)
[ 50.254172] [<c0868954>] (blk_add_partitions) from [<c05aa56c>]
(bdev_disk_changed+0x94/0x118)
[ 50.255630] [<c05aa56c>] (bdev_disk_changed) from [<c05ab140>]
(__blkdev_get+0x460/0x748)
[ 50.257091] [<c05ab140>] (__blkdev_get) from [<c08649b4>]
(__device_add_disk+0x718/0x808)
[ 50.258649] [<c08649b4>] (__device_add_disk) from [<c24b12ac>]
(brd_init+0x158/0x234)
[ 50.259989] [<c24b12ac>] (brd_init) from [<c0302630>]
(do_one_initcall+0xb4/0x30c)
[ 50.261275] [<c0302630>] (do_one_initcall) from [<c2401360>]
(kernel_init_freeable+0x1f8/0x26c)
[ 50.262629] [<c2401360>] (kernel_init_freeable) from [<c1545040>]
(kernel_init+0x8/0x120)
[ 50.263953] [<c1545040>] (kernel_init) from [<c03001b0>]
(ret_from_fork+0x14/0x24)
[ 50.265225] Exception stack(0xe88c3fb0 to 0xe88c3ff8)
[ 50.266569] 3fa0: 00000000
00000000 00000000 00000000
[ 50.268334] 3fc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[ 50.269970] 3fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[ 50.271241]
[ 50.271821]
[ 50.272268] Memory state around the buggy address:
[ 50.274111] f08cbd00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 50.275302] f08cbd80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 50.276296] >f08cbe00: 00 00 00 00 f1 f1 f1 f1 04 f2 04 f2 00 f3 f3 f3
[ 50.277548] ^
[ 50.278206] f08cbe80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 50.279194] f08cbf00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 50.280193] ==================================================================
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings
From: Florian Fainelli @ 2020-06-04 16:56 UTC (permalink / raw)
To: Stefan Wahren, Lukas Wunner
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Scott Branden, Ray Jui, Mark Brown, linux-kernel,
open list:SPI SUBSYSTEM, Nicolas Saenz Julienne, Rob Herring,
maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Martin Sperl,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE
In-Reply-To: <2978874a-fe1e-3b07-381d-55dcb00ecca7@i2se.com>
On 6/4/2020 9:54 AM, Stefan Wahren wrote:
> Am 04.06.20 um 18:40 schrieb Florian Fainelli:
>>
>> On 6/3/2020 9:20 PM, Lukas Wunner wrote:
>>> On Wed, Jun 03, 2020 at 08:46:54PM -0700, Florian Fainelli wrote:
>>>> The BCM2711 SoC features 5 SPI controllers which all share the same
>>>> interrupt line, the SPI driver needs to support interrupt sharing,
>>>> therefore use the chip specific compatible string to help with that.
>>> You're saying above that the 5 controllers all share the interrupt
>>> but below you're only changing the compatible string of 4 controllers.
>>>
>>> So I assume spi0 still has its own interrupt and only the additional
>>> 4 controllers present on the BCM2711/BCM7211 share their interrupt?
>> Correct, there are 5 instances, but only the 4 that were added for 2711
>> actually share the interrupt line, I will correct that in the next patch
>> version.
>
> No, all 5 instances uses the same interrupt line. Please see my comment
> before.
OK, but this is not going to be needed, I have another solution that
does not involve device tree changes.
--
Florian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings
From: Stefan Wahren @ 2020-06-04 16:54 UTC (permalink / raw)
To: Florian Fainelli, Lukas Wunner
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Scott Branden, Ray Jui, Mark Brown, linux-kernel,
open list:SPI SUBSYSTEM, Nicolas Saenz Julienne, Rob Herring,
maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Martin Sperl,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE
In-Reply-To: <15c3995e-87de-0f2b-3424-5dd698b181d3@gmail.com>
Am 04.06.20 um 18:40 schrieb Florian Fainelli:
>
> On 6/3/2020 9:20 PM, Lukas Wunner wrote:
>> On Wed, Jun 03, 2020 at 08:46:54PM -0700, Florian Fainelli wrote:
>>> The BCM2711 SoC features 5 SPI controllers which all share the same
>>> interrupt line, the SPI driver needs to support interrupt sharing,
>>> therefore use the chip specific compatible string to help with that.
>> You're saying above that the 5 controllers all share the interrupt
>> but below you're only changing the compatible string of 4 controllers.
>>
>> So I assume spi0 still has its own interrupt and only the additional
>> 4 controllers present on the BCM2711/BCM7211 share their interrupt?
> Correct, there are 5 instances, but only the 4 that were added for 2711
> actually share the interrupt line, I will correct that in the next patch
> version.
No, all 5 instances uses the same interrupt line. Please see my comment
before.
Regards
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings
From: Stefan Wahren @ 2020-06-04 16:46 UTC (permalink / raw)
To: Florian Fainelli, linux-kernel
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Scott Branden, lukas, Ray Jui, Mark Brown,
open list:SPI SUBSYSTEM, Nicolas Saenz Julienne, Rob Herring,
maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Martin Sperl,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE
In-Reply-To: <20200604034655.15930-3-f.fainelli@gmail.com>
Hi Florian,
Am 04.06.20 um 05:46 schrieb Florian Fainelli:
> The BCM2711 SoC features 5 SPI controllers which all share the same
> interrupt line, the SPI driver needs to support interrupt sharing,
> therefore use the chip specific compatible string to help with that.
the commit message is correct about 5 SPI controllers, but the patch
only changes 4 ones.
Please add the new compatibles also for &spi (included from
bcm283x.dtsi) below in this file, which also share interrupt 118.
Thanks
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings
From: Florian Fainelli @ 2020-06-04 16:40 UTC (permalink / raw)
To: Lukas Wunner, Florian Fainelli
Cc: moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Scott Branden, Ray Jui, linux-kernel, Rob Herring,
open list:SPI SUBSYSTEM, Mark Brown,
maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Martin Sperl, Nicolas Saenz Julienne
In-Reply-To: <20200604042038.jzolu6k7q3d6bsvq@wunner.de>
On 6/3/2020 9:20 PM, Lukas Wunner wrote:
> On Wed, Jun 03, 2020 at 08:46:54PM -0700, Florian Fainelli wrote:
>> The BCM2711 SoC features 5 SPI controllers which all share the same
>> interrupt line, the SPI driver needs to support interrupt sharing,
>> therefore use the chip specific compatible string to help with that.
>
> You're saying above that the 5 controllers all share the interrupt
> but below you're only changing the compatible string of 4 controllers.
>
> So I assume spi0 still has its own interrupt and only the additional
> 4 controllers present on the BCM2711/BCM7211 share their interrupt?
Correct, there are 5 instances, but only the 4 that were added for 2711
actually share the interrupt line, I will correct that in the next patch
version.
--
Florian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken
From: Julia Lawall @ 2020-06-04 16:29 UTC (permalink / raw)
To: Joe Perches
Cc: Linux ARM, Linus Walleij, kernel-janitors,
linux-kernel@vger.kernel.org, Haojian Zhuang, Julia Lawall,
open list:GPIO SUBSYSTEM, Christophe JAILLET, Dan Carpenter,
Robert Jarzmik, Daniel Mack
In-Reply-To: <0749ac5e3868c6ba50728ced8366bfd86b0b8500.camel@perches.com>
On Thu, 4 Jun 2020, Joe Perches wrote:
> On Thu, 2020-06-04 at 15:30 +0300, Dan Carpenter wrote:
> > On Thu, Jun 04, 2020 at 01:42:12PM +0200, Julia Lawall wrote:
> > > OK, I recall a discussion with Dan where he suggested that some things
> > > that were not actually bug fixes could also merit a Fixes tag. But it's
> > > probably better if he weighs in directly.
> >
> > I generally think Fixes should only be used for "real bug" fixes.
> >
> > The one exception is when I'm reviewing a patch that fixes an "unused
> > assignment" static checker warning is that I know which commit
> > introduced the warning. I don't have strong feelings if it's in the
> > Fixes tag or if it's just mentioned in the commit message.
>
> My view is that changes that silence compiler warnings are
> not fixing bugs and that these changes should generally not
> be backported.
>
> Compiler silencing changes marked as fixes can introduce other
> defects in working code.
>
> Backporting patches to stable trees should be conservatively
> rather than liberally applied.
>
> It seems that the actual backport maintainers disagree though.
But the rule seemed to be "bug fixing patches must contain a Fixes", and
not "backportable patches must contain a Fixes".
Overall, the relationship between backporting and Fixes is not so clear.
Patches that remove something unnecessary might benefit from having a
Fixes, but might not be worth backporting.
julia
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox