Hi Krzysztof,
On 7/11/2024 7:12 PM, Krzysztof Kozlowski wrote:
On 11/07/2024 13:08, Depeng Shao wrote:
+ */
+static int csid_reset(struct csid_device *csid)
+{
+ unsigned long time;
+ u32 val;
+ int i;
+
+ reinit_completion(&csid->reset_complete);
+
+ writel_relaxed(1, csid->base + CSID_TOP_IRQ_CLEAR);
+ writel_relaxed(1, csid->base + CSID_IRQ_CMD);
+ writel_relaxed(1, csid->base + CSID_TOP_IRQ_MASK);
+
+ for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
+ if (csid->phy.en_vc & BIT(i)) {
+ writel_relaxed(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
+ csid->base + CSID_BUF_DONE_IRQ_CLEAR);
+ writel_relaxed(0x1 << IRQ_CMD_CLEAR, csid->base + CSID_IRQ_CMD);
+ writel_relaxed(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
+ csid->base + CSID_BUF_DONE_IRQ_MASK);
+ }
+
+ /* preserve registers */
+ val = (0x1 << RST_LOCATION) | (0x1 << RST_MODE);
+ writel_relaxed(val, csid->base + CSID_RST_CFG);
... here - using everywhere relaxed here is odd and looks racy. These
looks like some strict sequences.
Yes, these are some sequences to initialize the HW.
Hm? It's like you ignore the problem and just answer with whatever to
shut up the reviewer. Instead of replying with the same, address the
problem. Why ordering is not a problem here?
Sorry, I didn't mean that, was trying to understand the problem, then
just sent out the mail by mistake.
Do you mean we should use writel to ensure the strict sequences?
Thanks for catching this problem, this problem is also in the the
existing camss driver. I will check all of them in this series, but the
problem in some existing camss drivers, maybe Bryan from Linaro can help
to fix them, since I don't have these devices to verify the modifications.
+
+const struct csid_hw_ops csid_ops_gen3 = {
Isn't there a warning here?
+ .configure_stream = csid_configure_stream,
+ .configure_testgen_pattern = csid_configure_testgen_pattern,
+ .hw_version = csid_hw_version,
+ .isr = csid_isr,
+ .reset = csid_reset,
+ .src_pad_code = csid_src_pad_code,
+ .subdev_init = csid_subdev_init,
+};
Your patchset does not apply at all. Tried v6.9, v6.10, next. I see some
dependency above, but that means no one can test it and no one can apply it.
Fix the warnings, I cannot verify it but I am sure you have them.
My code base is next master branch, do you mean the 'declared and not
used' warning? I don't see this warning with below two version compiler
even though I just pick this patch and pull the code the latest version.
But it indeed have this issue, these structures are declared and will be
used later in "media: qcom: camss: Add sm8550 resources" patch, will
think about how to avoid this.
aarch64-linux-gnu-gcc (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04) 7.5.0
aarch64-linux-gnu-gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0
That's some old compilers... I am talking about recent GCC, recent clang
and of course W=1.
Thanks for the sharing, I will try to upgrade to latest compiler to
avoid other potential issues.
Thanks,
Depeng
