| Message ID | 20240709160656.31146-1-quic_depengs@quicinc.com |
|---|---|
| Headers | show |
| Series | media: qcom: camss: Add sm8550 support | expand |
On 09/07/2024 18:06, Depeng Shao wrote: > V3: > - Rebased the change based on below change which will be merged firstly. > "Move camss version related defs in to resources" > Link: https://lore.kernel.org/all/20240522154659.510-1-quic_grosikop@quicinc.com/ > - Rebased the change based on Bryan's csiphy optimization change and add > these changes into this series, so that the new csiphy-3ph driver don't > need to add duplicate code. This has got Bryan's permission to add his > patches into this series. > - Refactor some changes based on the comments to move the random code to > patches where they are used. > - Remove the vfe780 irq function since it isn't doing the actual work. > - Add dt-binding for sm8550 camss driver. > Link to V2: https://lore.kernel.org/all/20240320141136.26827-1-quic_depengs@quicinc.com/ I asked for reference to upstream DTS - where can I find the DTS patches? Best regards, Krzysztof
On 09/07/2024 18:06, Depeng Shao wrote: > The CSID in SM8550 is gen3, it has new register offset and new > functionality. The buf done irq,register update and reset are > moved to CSID gen3. And CSID gen3 has a new register block which > is named as CSID top, it controls the output of CSID, since the > CSID can connect to Sensor Front End (SFE) or original VFE, the > register in top block is used to control the HW connection. > > Co-developed-by: Yongsheng Li <quic_yon@quicinc.com> > Signed-off-by: Yongsheng Li <quic_yon@quicinc.com> > Signed-off-by: Depeng Shao <quic_depengs@quicinc.com> > --- > drivers/media/platform/qcom/camss/Makefile | 1 + > .../platform/qcom/camss/camss-csid-gen3.c | 445 ++++++++++++++++++ > .../platform/qcom/camss/camss-csid-gen3.h | 26 + > .../media/platform/qcom/camss/camss-csid.h | 2 + > 4 files changed, 474 insertions(+) > create mode 100644 drivers/media/platform/qcom/camss/camss-csid-gen3.c > create mode 100644 drivers/media/platform/qcom/camss/camss-csid-gen3.h > > diff --git a/drivers/media/platform/qcom/camss/Makefile b/drivers/media/platform/qcom/camss/Makefile > index e636968a1126..c336e4c1a399 100644 > --- a/drivers/media/platform/qcom/camss/Makefile > +++ b/drivers/media/platform/qcom/camss/Makefile > @@ -7,6 +7,7 @@ qcom-camss-objs += \ > camss-csid-4-1.o \ > camss-csid-4-7.o \ > camss-csid-gen2.o \ > + camss-csid-gen3.o \ > camss-csiphy-2ph-1-0.o \ > camss-csiphy-3ph-1-0.o \ > camss-csiphy.o \ > diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen3.c b/drivers/media/platform/qcom/camss/camss-csid-gen3.c > new file mode 100644 > index 000000000000..17fd7c5499de > --- /dev/null > +++ b/drivers/media/platform/qcom/camss/camss-csid-gen3.c > @@ -0,0 +1,445 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * camss-csid-gen3.c > + * > + * Qualcomm MSM Camera Subsystem - CSID (CSI Decoder) Module > + * > + * Copyright (c) 2024 Qualcomm Technologies, Inc. > + */ > +#include <linux/completion.h> > +#include <linux/delay.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/of.h> > + > +#include "camss.h" > +#include "camss-csid.h" > +#include "camss-csid-gen3.h" > + > + > +#define CSID_TOP_IO_PATH_CFG0(csid) (0x4 * (csid)) > +#define OUTPUT_IFE_EN 0x100 > +#define INTERNAL_CSID 1 > + > +#define CSID_HW_VERSION 0x0 > +#define HW_VERSION_STEPPING 0 > +#define HW_VERSION_REVISION 16 > +#define HW_VERSION_GENERATION 28 > + > +#define CSID_RST_CFG 0xC > +#define RST_MODE 0 > +#define RST_LOCATION 4 > + > +#define CSID_RST_CMD 0x10 > +#define SELECT_HW_RST 0 > +#define SELECT_SW_RST 1 > +#define SELECT_IRQ_RST 2 > + > +#define CSID_CSI2_RX_IRQ_STATUS 0x9C > +#define CSID_CSI2_RX_IRQ_MASK 0xA0 > +#define CSID_CSI2_RX_IRQ_CLEAR 0xA4 > +#define CSID_CSI2_RX_IRQ_SET 0xA8 > + > +#define CSID_CSI2_RDIN_IRQ_STATUS(rdi) (0xEC + 0x10 * (rdi)) > +#define CSID_CSI2_RDIN_IRQ_MASK(rdi) (0xF0 + 0x10 * (rdi)) > +#define CSID_CSI2_RDIN_INFO_FIFO_FULL 2 That's a random set of indentations. > +#define CSID_CSI2_RDIN_INFO_CAMIF_EOF 3 > +#define CSID_CSI2_RDIN_INFO_CAMIF_SOF 4 > +#define CSID_CSI2_RDIN_INFO_INPUT_EOF 9 > +#define CSID_CSI2_RDIN_INFO_INPUT_SOF 12 ... > + > + writel_relaxed(val, csid->base + CSID_RDI_CFG0(vc)); > + > + val = 1 << RDI_CFG1_PACKING_FORMAT; > + val |= 1 << RDI_CFG1_PIX_STORE; > + val |= 1 << RDI_CFG1_DROP_H_EN; > + val |= 1 << RDI_CFG1_DROP_V_EN; > + val |= 1 << RDI_CFG1_CROP_H_EN; > + val |= 1 << RDI_CFG1_CROP_V_EN; > + val |= RDI_CFG1_EARLY_EOF_EN; > + > + writel_relaxed(val, csid->base + CSID_RDI_CFG1(vc)); > + > + val = 0; > + writel_relaxed(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PERIOD(vc)); > + > + val = 1; > + writel_relaxed(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PATTERN(vc)); > + > + val = 0; > + writel_relaxed(val, csid->base + CSID_RDI_CTRL(vc)); > + > + val = readl_relaxed(csid->base + CSID_RDI_CFG0(vc)); > + val |= enable << RDI_CFG0_EN; > + writel_relaxed(val, csid->base + CSID_RDI_CFG0(vc)); > +} > + such patterns and... > + */ > +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. > + > + val = (0x1 << SELECT_HW_RST) | (0x1 << SELECT_IRQ_RST); > + writel_relaxed(val, csid->base + CSID_RST_CMD); > + > + time = wait_for_completion_timeout(&csid->reset_complete, > + msecs_to_jiffies(CSID_RESET_TIMEOUT_MS)); > + if (!time) { > + dev_err(csid->camss->dev, "CSID reset timeout\n"); > + return -EIO; > + } > + > + > +static void csid_subdev_init(struct csid_device *csid) > +{ > + csid->testgen.modes = csid_testgen_modes; > + csid->testgen.nmodes = CSID_PAYLOAD_MODE_NUM_SUPPORTED_GEN2; > +} > + > +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. Best regards, Krzysztof
On 7/10/2024 7:08 PM, Krzysztof Kozlowski wrote: > On 09/07/2024 18:06, Depeng Shao wrote: >> V3: >> - Rebased the change based on below change which will be merged firstly. >> "Move camss version related defs in to resources" >> Link: https://lore.kernel.org/all/20240522154659.510-1-quic_grosikop@quicinc.com/ >> - Rebased the change based on Bryan's csiphy optimization change and add >> these changes into this series, so that the new csiphy-3ph driver don't >> need to add duplicate code. This has got Bryan's permission to add his >> patches into this series. >> - Refactor some changes based on the comments to move the random code to >> patches where they are used. >> - Remove the vfe780 irq function since it isn't doing the actual work. >> - Add dt-binding for sm8550 camss driver. >> Link to V2: https://lore.kernel.org/all/20240320141136.26827-1-quic_depengs@quicinc.com/ > > I asked for reference to upstream DTS - where can I find the DTS patches? > > Best regards, > Krzysztof > Hi Krzysztof, Sorry for that, I thought add the dt-binding is also fine, since I saw other patches also do like this. Will add add the DTS in next patch set. Thanks, Depeng
On 09/07/2024 17:06, Depeng Shao wrote: > The main v4l2 process logic in camss vfe subdev driver, so the vfe driver > handles the buf done irq and register update configuration. But the buf > done irq and register update configuration have been moved CSID HW in > Titan 780 and other new platform, so vfe driver needs to call into CSID > driver to configure the register update. And CSID driver also needs to > call into vfe driver to notify of the buf done irq. > > Adding this notify interface in camss structure to do the subdevs cross > communication to decouple CSID and VFE, the subdevs can add an interface > to process the message what is routed from other subdevices, then we can > process the cross communication easily. > > Signed-off-by: Depeng Shao <quic_depengs@quicinc.com> > --- > .../platform/qcom/camss/camss-csid-gen3.c | 38 +++++++++++++++ > .../media/platform/qcom/camss/camss-csid.h | 8 ++++ > .../media/platform/qcom/camss/camss-vfe-780.c | 29 +++++++++++- > drivers/media/platform/qcom/camss/camss-vfe.h | 1 + > drivers/media/platform/qcom/camss/camss.c | 47 +++++++++++++++++++ > drivers/media/platform/qcom/camss/camss.h | 9 ++++ > 6 files changed, 130 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen3.c b/drivers/media/platform/qcom/camss/camss-csid-gen3.c > index 17fd7c5499de..585054948255 100644 > --- a/drivers/media/platform/qcom/camss/camss-csid-gen3.c > +++ b/drivers/media/platform/qcom/camss/camss-csid-gen3.c > @@ -312,6 +312,18 @@ static u32 csid_hw_version(struct csid_device *csid) > return hw_version; > } > > +static void csid_reg_update(struct csid_device *csid, int port_id) > +{ > + csid->reg_update |= REG_UPDATE_RDI(csid, port_id); > + writel_relaxed(csid->reg_update, csid->base + CSID_REG_UPDATE_CMD); > +} > + > +static inline void csid_reg_update_clear(struct csid_device *csid, > + int port_id) > +{ > + csid->reg_update &= ~REG_UPDATE_RDI(csid, port_id); > +} > + > /* > * csid_isr - CSID module interrupt service routine > * @irq: Interrupt line > @@ -341,6 +353,14 @@ static irqreturn_t csid_isr(int irq, void *dev) > if (csid->phy.en_vc & BIT(i)) { > val = readl_relaxed(csid->base + CSID_CSI2_RDIN_IRQ_STATUS(i)); > writel_relaxed(val, csid->base + CSID_CSI2_RDIN_IRQ_CLEAR(i)); > + > + if (buf_done_val & BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i)) { > + /* For Titan 780, Buf Done IRQ® has been moved to CSID from VFE. > + * Once CSID received Buf Done, need notify this event to VFE. > + * Trigger VFE to handle Buf Done process. > + */ > + csid->camss->notify(&csid->subdev, CAMSS_MSG_BUF_DONE, (void *)&i); Instead of a generic notify function with an enum passed to a switch lets just have a dedicated function for each functional callback. csid->camss->reg_update_cmd(); csid->camss->reg_clear_cmd(); We can get rid of a switch and an additional enum that way, plus redundant "event not supported" error checking. > + } > } > > val = 1 << IRQ_CMD_CLEAR; > @@ -434,6 +454,23 @@ static void csid_subdev_init(struct csid_device *csid) > csid->testgen.nmodes = CSID_PAYLOAD_MODE_NUM_SUPPORTED_GEN2; > } > > +static void csid_subdev_process_msg(struct csid_device *csid, unsigned int msg_type, void *arg) > +{ > + int msg_data = *(int *)arg; > + > + switch (msg_type) { > + case CAMSS_MSG_RUP: > + csid_reg_update(csid, msg_data); > + break; > + case CAMSS_MSG_RUP_CLEAR: > + csid_reg_update_clear(csid, msg_data); > + break; > + default: > + dev_err(csid->camss->dev, "NOT Supported EVT Type\n"); > + break; > + } > +} > + > const struct csid_hw_ops csid_ops_gen3 = { > .configure_stream = csid_configure_stream, > .configure_testgen_pattern = csid_configure_testgen_pattern, > @@ -442,4 +479,5 @@ const struct csid_hw_ops csid_ops_gen3 = { > .reset = csid_reset, > .src_pad_code = csid_src_pad_code, > .subdev_init = csid_subdev_init, > + .process_msg = csid_subdev_process_msg, > }; > diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h > index ae5b6b0dc0ea..714a8db855fd 100644 > --- a/drivers/media/platform/qcom/camss/camss-csid.h > +++ b/drivers/media/platform/qcom/camss/camss-csid.h > @@ -152,6 +152,14 @@ struct csid_hw_ops { > * @csid: CSID device > */ > void (*subdev_init)(struct csid_device *csid); > + > + /* > + * process_msg - receive message from other sub device > + * @csid: CSID device > + * @evt_type: event type > + * @arg: arguments > + */ > + void (*process_msg)(struct csid_device *csid, unsigned int evt_type, void *arg); > }; > > struct csid_subdev_resources { > diff --git a/drivers/media/platform/qcom/camss/camss-vfe-780.c b/drivers/media/platform/qcom/camss/camss-vfe-780.c > index abef2d5b9c2e..3279fe53b987 100644 > --- a/drivers/media/platform/qcom/camss/camss-vfe-780.c > +++ b/drivers/media/platform/qcom/camss/camss-vfe-780.c > @@ -131,13 +131,23 @@ static void vfe_wm_update(struct vfe_device *vfe, u8 wm, u64 addr, > > static void vfe_reg_update(struct vfe_device *vfe, enum vfe_line_id line_id) > { > - /* TODO: Add register update support */ > + int port_id = line_id; > + > + /* RUP(register update) registers has beem moved to CSID in Titan 780. > + * Notify the event of trigger RUP. > + */ > + vfe->camss->notify(&vfe->line[line_id].subdev, CAMSS_MSG_RUP, (void *)&port_id); > } > > static inline void vfe_reg_update_clear(struct vfe_device *vfe, > enum vfe_line_id line_id) > { > - /* TODO: Add register update clear support */ > + int port_id = line_id; > + > + /* RUP(register update) registers has beem moved to CSID in Titan 780. > + * Notify the event of trigger RUP clear. > + */ > + vfe->camss->notify(&vfe->line[line_id].subdev, CAMSS_MSG_RUP_CLEAR, (void *)&port_id); > } > > /* > @@ -390,6 +400,20 @@ static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe) > vfe->video_ops = vfe_video_ops_780; > } > > +static void vfe_subdev_process_msg(struct vfe_device *vfe, unsigned int msg_type, void *arg) > +{ > + int port_id = *(int *)arg; > + > + switch (msg_type) { > + case CAMSS_MSG_BUF_DONE: > + vfe_buf_done(vfe, port_id); > + break; > + > + default: > + break; > + } > +} > + > const struct vfe_hw_ops vfe_ops_780 = { > .global_reset = NULL, > .hw_version = vfe_hw_version, > @@ -401,4 +425,5 @@ const struct vfe_hw_ops vfe_ops_780 = { > .vfe_enable = vfe_enable, > .vfe_halt = vfe_halt, > .vfe_wm_stop = vfe_wm_stop, > + .process_msg = vfe_subdev_process_msg, > }; > diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h > index 10e2cc3c0b83..a8b09ce9941b 100644 > --- a/drivers/media/platform/qcom/camss/camss-vfe.h > +++ b/drivers/media/platform/qcom/camss/camss-vfe.h > @@ -115,6 +115,7 @@ struct vfe_hw_ops { > int (*vfe_halt)(struct vfe_device *vfe); > void (*violation_read)(struct vfe_device *vfe); > void (*vfe_wm_stop)(struct vfe_device *vfe, u8 wm); > + void (*process_msg)(struct vfe_device *vfe, unsigned int msg_type, void *arg); > }; > > struct vfe_isr_ops { > diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c > index 1f1f44f6fbb2..abeb0918e47d 100644 > --- a/drivers/media/platform/qcom/camss/camss.c > +++ b/drivers/media/platform/qcom/camss/camss.c > @@ -2202,6 +2202,52 @@ static void camss_genpd_cleanup(struct camss *camss) > dev_pm_domain_detach(camss->genpd, true); > } > > +static void camss_notify_msg(struct v4l2_subdev *sd, > + unsigned int msg_type, void *arg) > +{ > + struct v4l2_device *v4l2_dev = sd->v4l2_dev; > + struct camss *camss = to_camss(v4l2_dev); > + struct vfe_device *vfe; > + struct vfe_line *vfe_line; > + struct csid_device *csid; > + int evt_data = *(int *)arg; > + > + switch (msg_type) { > + case CAMSS_MSG_BUF_DONE: > + csid = v4l2_get_subdevdata(sd); > + vfe = &(camss->vfe[csid->id]); > + if (vfe->res->hw_ops->process_msg) > + vfe->res->hw_ops->process_msg(vfe, > + CAMSS_MSG_BUF_DONE, (void *)&evt_data); > + break; > + > + case CAMSS_MSG_RUP: > + vfe_line = v4l2_get_subdevdata(sd); > + vfe = to_vfe(vfe_line); > + csid = &(camss->csid[vfe->id]); > + > + if (csid->res->hw_ops->process_msg) > + csid->res->hw_ops->process_msg(csid, > + CAMSS_MSG_RUP, (void *)&evt_data); > + break; > + > + case CAMSS_MSG_RUP_CLEAR: > + vfe_line = v4l2_get_subdevdata(sd); > + vfe = to_vfe(vfe_line); > + csid = &(camss->csid[vfe->id]); > + > + if (csid->res->hw_ops->process_msg) > + csid->res->hw_ops->process_msg(csid, > + CAMSS_MSG_RUP_CLEAR, (void *)&evt_data); > + > + break; > + > + default: > + dev_err(camss->dev, "Not supported evt type\n"); > + break; > + } > +} Instead of having a central swiss army knife notify() callback these should be CSID or VFE specific callbacks vfe->buf_done() csid->msg_rup() etc. --- bod
On 10/07/2024 13:27, Depeng Shao wrote: > > > On 7/10/2024 7:08 PM, Krzysztof Kozlowski wrote: >> On 09/07/2024 18:06, Depeng Shao wrote: >>> V3: >>> - Rebased the change based on below change which will be merged firstly. >>> "Move camss version related defs in to resources" >>> Link: https://lore.kernel.org/all/20240522154659.510-1-quic_grosikop@quicinc.com/ >>> - Rebased the change based on Bryan's csiphy optimization change and add >>> these changes into this series, so that the new csiphy-3ph driver don't >>> need to add duplicate code. This has got Bryan's permission to add his >>> patches into this series. >>> - Refactor some changes based on the comments to move the random code to >>> patches where they are used. >>> - Remove the vfe780 irq function since it isn't doing the actual work. >>> - Add dt-binding for sm8550 camss driver. >>> Link to V2: https://lore.kernel.org/all/20240320141136.26827-1-quic_depengs@quicinc.com/ >> >> I asked for reference to upstream DTS - where can I find the DTS patches? >> >> Best regards, >> Krzysztof >> > > Hi Krzysztof, > > Sorry for that, I thought add the dt-binding is also fine, since I saw > other patches also do like this. Will add add the DTS in next patch set. DTS should not be part of this patchset, but sent separately. It's enough if you post a lore link to it. Best regards, Krzysztof
Hi Krzysztof, On 7/10/2024 7:20 PM, Krzysztof Kozlowski wrote: > On 09/07/2024 18:06, Depeng Shao wrote: >> The CSID in SM8550 is gen3, it has new register offset and new >> functionality. The buf done irq,register update and reset are >> moved to CSID gen3. And CSID gen3 has a new register block which >> is named as CSID top, it controls the output of CSID, since the >> CSID can connect to Sensor Front End (SFE) or original VFE, the >> register in top block is used to control the HW connection. >> >> Co-developed-by: Yongsheng Li <quic_yon@quicinc.com> >> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com> >> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com> >> --- >> drivers/media/platform/qcom/camss/Makefile | 1 + >> .../platform/qcom/camss/camss-csid-gen3.c | 445 ++++++++++++++++++ >> .../platform/qcom/camss/camss-csid-gen3.h | 26 + >> .../media/platform/qcom/camss/camss-csid.h | 2 + >> 4 files changed, 474 insertions(+) >> create mode 100644 drivers/media/platform/qcom/camss/camss-csid-gen3.c >> create mode 100644 drivers/media/platform/qcom/camss/camss-csid-gen3.h >> >> diff --git a/drivers/media/platform/qcom/camss/Makefile b/drivers/media/platform/qcom/camss/Makefile >> index e636968a1126..c336e4c1a399 100644 >> --- a/drivers/media/platform/qcom/camss/Makefile >> +++ b/drivers/media/platform/qcom/camss/Makefile >> @@ -7,6 +7,7 @@ qcom-camss-objs += \ >> camss-csid-4-1.o \ >> camss-csid-4-7.o \ >> camss-csid-gen2.o \ >> + camss-csid-gen3.o \ >> camss-csiphy-2ph-1-0.o \ >> camss-csiphy-3ph-1-0.o \ >> camss-csiphy.o \ >> diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen3.c b/drivers/media/platform/qcom/camss/camss-csid-gen3.c >> new file mode 100644 >> index 000000000000..17fd7c5499de >> --- /dev/null >> +++ b/drivers/media/platform/qcom/camss/camss-csid-gen3.c >> @@ -0,0 +1,445 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * camss-csid-gen3.c >> + * >> + * Qualcomm MSM Camera Subsystem - CSID (CSI Decoder) Module >> + * >> + * Copyright (c) 2024 Qualcomm Technologies, Inc. >> + */ >> +#include <linux/completion.h> >> +#include <linux/delay.h> >> +#include <linux/interrupt.h> >> +#include <linux/io.h> >> +#include <linux/kernel.h> >> +#include <linux/of.h> >> + >> +#include "camss.h" >> +#include "camss-csid.h" >> +#include "camss-csid-gen3.h" >> + >> + >> +#define CSID_TOP_IO_PATH_CFG0(csid) (0x4 * (csid)) >> +#define OUTPUT_IFE_EN 0x100 >> +#define INTERNAL_CSID 1 >> + >> +#define CSID_HW_VERSION 0x0 >> +#define HW_VERSION_STEPPING 0 >> +#define HW_VERSION_REVISION 16 >> +#define HW_VERSION_GENERATION 28 >> + >> +#define CSID_RST_CFG 0xC >> +#define RST_MODE 0 >> +#define RST_LOCATION 4 >> + >> +#define CSID_RST_CMD 0x10 >> +#define SELECT_HW_RST 0 >> +#define SELECT_SW_RST 1 >> +#define SELECT_IRQ_RST 2 >> + >> +#define CSID_CSI2_RX_IRQ_STATUS 0x9C >> +#define CSID_CSI2_RX_IRQ_MASK 0xA0 >> +#define CSID_CSI2_RX_IRQ_CLEAR 0xA4 >> +#define CSID_CSI2_RX_IRQ_SET 0xA8 >> + >> +#define CSID_CSI2_RDIN_IRQ_STATUS(rdi) (0xEC + 0x10 * (rdi)) >> +#define CSID_CSI2_RDIN_IRQ_MASK(rdi) (0xF0 + 0x10 * (rdi)) >> +#define CSID_CSI2_RDIN_INFO_FIFO_FULL 2 > > That's a random set of indentations. > Thanks, will fixes this. >> +#define CSID_CSI2_RDIN_INFO_CAMIF_EOF 3 >> +#define CSID_CSI2_RDIN_INFO_CAMIF_SOF 4 >> +#define CSID_CSI2_RDIN_INFO_INPUT_EOF 9 >> +#define CSID_CSI2_RDIN_INFO_INPUT_SOF 12 > > > ... > >> + >> + writel_relaxed(val, csid->base + CSID_RDI_CFG0(vc)); >> + >> + val = 1 << RDI_CFG1_PACKING_FORMAT; >> + val |= 1 << RDI_CFG1_PIX_STORE; >> + val |= 1 << RDI_CFG1_DROP_H_EN; >> + val |= 1 << RDI_CFG1_DROP_V_EN; >> + val |= 1 << RDI_CFG1_CROP_H_EN; >> + val |= 1 << RDI_CFG1_CROP_V_EN; >> + val |= RDI_CFG1_EARLY_EOF_EN; >> + >> + writel_relaxed(val, csid->base + CSID_RDI_CFG1(vc)); >> + >> + val = 0; >> + writel_relaxed(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PERIOD(vc)); >> + >> + val = 1; >> + writel_relaxed(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PATTERN(vc)); >> + >> + val = 0; >> + writel_relaxed(val, csid->base + CSID_RDI_CTRL(vc)); >> + >> + val = readl_relaxed(csid->base + CSID_RDI_CFG0(vc)); >> + val |= enable << RDI_CFG0_EN; >> + writel_relaxed(val, csid->base + CSID_RDI_CFG0(vc)); >> +} >> + > > such patterns and... > >> + */ >> +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. > >> + >> + val = (0x1 << SELECT_HW_RST) | (0x1 << SELECT_IRQ_RST); >> + writel_relaxed(val, csid->base + CSID_RST_CMD); >> + >> + time = wait_for_completion_timeout(&csid->reset_complete, >> + msecs_to_jiffies(CSID_RESET_TIMEOUT_MS)); >> + if (!time) { >> + dev_err(csid->camss->dev, "CSID reset timeout\n"); >> + return -EIO; >> + } >> + > > >> + >> +static void csid_subdev_init(struct csid_device *csid) >> +{ >> + csid->testgen.modes = csid_testgen_modes; >> + csid->testgen.nmodes = CSID_PAYLOAD_MODE_NUM_SUPPORTED_GEN2; >> +} >> + >> +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 --- CALL scripts/checksyscalls.sh CC [M] drivers/media/platform/qcom/camss/camss-csid.o CC [M] drivers/media/platform/qcom/camss/camss-csid-4-1.o CC [M] drivers/media/platform/qcom/camss/camss-csid-4-7.o CC [M] drivers/media/platform/qcom/camss/camss-csid-gen2.o CC [M] drivers/media/platform/qcom/camss/camss-csid-gen3.o CC [M] drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.o CC [M] drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.o --- 89a4a25b7b56 (HEAD -> master) media: qcom: camss: Add CSID Gen3 support for SM8550 d0cfd24496d3 media: qcom: camss: csiphy-3ph: Add Gen2 v1.2 two-phase MIPI CSI-2 DPHY init 5795fd39a8ee dt-bindings: media: camss: Add qcom,sm8550-camss binding > Best regards, > Krzysztof > Thanks, Depeng
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? > >> >>> + >>> + val = (0x1 << SELECT_HW_RST) | (0x1 << SELECT_IRQ_RST); >>> + writel_relaxed(val, csid->base + CSID_RST_CMD); >>> + >>> + time = wait_for_completion_timeout(&csid->reset_complete, >>> + msecs_to_jiffies(CSID_RESET_TIMEOUT_MS)); >>> + if (!time) { >>> + dev_err(csid->camss->dev, "CSID reset timeout\n"); >>> + return -EIO; >>> + } >>> + >> >> >>> + >>> +static void csid_subdev_init(struct csid_device *csid) >>> +{ >>> + csid->testgen.modes = csid_testgen_modes; >>> + csid->testgen.nmodes = CSID_PAYLOAD_MODE_NUM_SUPPORTED_GEN2; >>> +} >>> + >>> +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. Best regards, Krzysztof
Hi Krzysztof, On 7/10/2024 8:30 PM, Krzysztof Kozlowski wrote: > On 10/07/2024 13:27, Depeng Shao wrote: >> >> >> On 7/10/2024 7:08 PM, Krzysztof Kozlowski wrote: >>> On 09/07/2024 18:06, Depeng Shao wrote: >>>> V3: >>>> - Rebased the change based on below change which will be merged firstly. >>>> "Move camss version related defs in to resources" >>>> Link: https://lore.kernel.org/all/20240522154659.510-1-quic_grosikop@quicinc.com/ >>>> - Rebased the change based on Bryan's csiphy optimization change and add >>>> these changes into this series, so that the new csiphy-3ph driver don't >>>> need to add duplicate code. This has got Bryan's permission to add his >>>> patches into this series. >>>> - Refactor some changes based on the comments to move the random code to >>>> patches where they are used. >>>> - Remove the vfe780 irq function since it isn't doing the actual work. >>>> - Add dt-binding for sm8550 camss driver. >>>> Link to V2: https://lore.kernel.org/all/20240320141136.26827-1-quic_depengs@quicinc.com/ >>> >>> I asked for reference to upstream DTS - where can I find the DTS patches? >>> >>> Best regards, >>> Krzysztof >>> >> >> Hi Krzysztof, >> >> Sorry for that, I thought add the dt-binding is also fine, since I saw >> other patches also do like this. Will add add the DTS in next patch set. > > DTS should not be part of this patchset, but sent separately. It's > enough if you post a lore link to it. > Thanks for the explanation, I will post the link in new version series. > Best regards, > Krzysztof > Thanks, Depeng
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
Hi Bryan, On 7/10/2024 7:54 PM, Bryan O'Donoghue wrote: > On 09/07/2024 17:06, Depeng Shao wrote: >> The main v4l2 process logic in camss vfe subdev driver, so the vfe driver >> handles the buf done irq and register update configuration. But the buf >> done irq and register update configuration have been moved CSID HW in >> Titan 780 and other new platform, so vfe driver needs to call into CSID >> driver to configure the register update. And CSID driver also needs to >> call into vfe driver to notify of the buf done irq. >> >> Adding this notify interface in camss structure to do the subdevs cross >> /* >> * csid_isr - CSID module interrupt service routine >> * @irq: Interrupt line >> @@ -341,6 +353,14 @@ static irqreturn_t csid_isr(int irq, void *dev) >> if (csid->phy.en_vc & BIT(i)) { >> val = readl_relaxed(csid->base + >> CSID_CSI2_RDIN_IRQ_STATUS(i)); >> writel_relaxed(val, csid->base + >> CSID_CSI2_RDIN_IRQ_CLEAR(i)); >> + >> + if (buf_done_val & BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + >> i)) { >> + /* For Titan 780, Buf Done IRQ® has been moved to >> CSID from VFE. >> + * Once CSID received Buf Done, need notify this >> event to VFE. >> + * Trigger VFE to handle Buf Done process. >> + */ >> + csid->camss->notify(&csid->subdev, >> CAMSS_MSG_BUF_DONE, (void *)&i); > > Instead of a generic notify function with an enum passed to a switch > lets just have a dedicated function for each functional callback. > > csid->camss->reg_update_cmd(); > csid->camss->reg_clear_cmd(); > > We can get rid of a switch and an additional enum that way, plus > redundant "event not supported" error checking. > Sure, will update the code based on suggestion. >> + } >> } >> val = 1 << IRQ_CMD_CLEAR; >> @@ -434,6 +454,23 @@ static void csid_subdev_init(struct csid_device >> *csid) >> csid->testgen.nmodes = CSID_PAYLOAD_MODE_NUM_SUPPORTED_GEN2; >> } >> +static void csid_subdev_process_msg(struct csid_device *csid, >> unsigned int msg_type, void *arg) >> +{ >> + int msg_data = *(int *)arg; >> + >> + switch (msg_type) { >> + case CAMSS_MSG_RUP: >> + csid_reg_update(csid, msg_data); >> + break; >> + case CAMSS_MSG_RUP_CLEAR: >> + csid_reg_update_clear(csid, msg_data); >> + break; >> + default: >> + dev_err(csid->camss->dev, "NOT Supported EVT Type\n"); >> + break; >> + } >> +} >> + >> const struct csid_hw_ops csid_ops_gen3 = { >> .configure_stream = csid_configure_stream, >> .configure_testgen_pattern = csid_configure_testgen_pattern, >> @@ -442,4 +479,5 @@ const struct csid_hw_ops csid_ops_gen3 = { >> .reset = csid_reset, >> .src_pad_code = csid_src_pad_code, >> .subdev_init = csid_subdev_init, >> + .process_msg = csid_subdev_process_msg, >> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h >> b/drivers/media/platform/qcom/camss/camss-vfe.h >> index 10e2cc3c0b83..a8b09ce9941b 100644 >> --- a/drivers/media/platform/qcom/camss/camss-vfe.h >> +++ b/drivers/media/platform/qcom/camss/camss-vfe.h >> @@ -115,6 +115,7 @@ struct vfe_hw_ops { >> int (*vfe_halt)(struct vfe_device *vfe); >> void (*violation_read)(struct vfe_device *vfe); >> void (*vfe_wm_stop)(struct vfe_device *vfe, u8 wm); >> + void (*process_msg)(struct vfe_device *vfe, unsigned int >> msg_type, void *arg); >> }; >> struct vfe_isr_ops { >> diff --git a/drivers/media/platform/qcom/camss/camss.c >> b/drivers/media/platform/qcom/camss/camss.c >> index 1f1f44f6fbb2..abeb0918e47d 100644 >> --- a/drivers/media/platform/qcom/camss/camss.c >> +++ b/drivers/media/platform/qcom/camss/camss.c >> @@ -2202,6 +2202,52 @@ static void camss_genpd_cleanup(struct camss >> *camss) >> dev_pm_domain_detach(camss->genpd, true); >> } >> +static void camss_notify_msg(struct v4l2_subdev *sd, >> + unsigned int msg_type, void *arg) >> +{ >> + struct v4l2_device *v4l2_dev = sd->v4l2_dev; >> + struct camss *camss = to_camss(v4l2_dev); >> + struct vfe_device *vfe; >> + struct vfe_line *vfe_line; >> + struct csid_device *csid; >> + int evt_data = *(int *)arg; >> + >> + switch (msg_type) { >> + case CAMSS_MSG_BUF_DONE: >> + csid = v4l2_get_subdevdata(sd); >> + vfe = &(camss->vfe[csid->id]); >> + if (vfe->res->hw_ops->process_msg) >> + vfe->res->hw_ops->process_msg(vfe, >> + CAMSS_MSG_BUF_DONE, (void *)&evt_data); >> + break; >> + >> + case CAMSS_MSG_RUP: >> + vfe_line = v4l2_get_subdevdata(sd); >> + vfe = to_vfe(vfe_line); >> + csid = &(camss->csid[vfe->id]); >> + >> + if (csid->res->hw_ops->process_msg) >> + csid->res->hw_ops->process_msg(csid, >> + CAMSS_MSG_RUP, (void *)&evt_data); >> + break; >> + >> + case CAMSS_MSG_RUP_CLEAR: >> + vfe_line = v4l2_get_subdevdata(sd); >> + vfe = to_vfe(vfe_line); >> + csid = &(camss->csid[vfe->id]); >> + >> + if (csid->res->hw_ops->process_msg) >> + csid->res->hw_ops->process_msg(csid, >> + CAMSS_MSG_RUP_CLEAR, (void *)&evt_data); >> + >> + break; >> + >> + default: >> + dev_err(camss->dev, "Not supported evt type\n"); >> + break; >> + } >> +} > > Instead of having a central swiss army knife notify() callback these > should be CSID or VFE specific callbacks vfe->buf_done() csid->msg_rup() > etc. > I will try to add the callback in CSID and VFE driver. Thanks, Depeng
On 11/07/2024 12:41, Depeng Shao wrote: >>> 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. _relaxed is used I'm sure because that's what's always been used and what downstream does. Is there a good reason for it ? None that I can think of. Krzysztof is right, there's no good reason to use relaxed() here at all, you should drop it. --- bod
Hi Bryan, On 7/11/2024 8:00 PM, Bryan O'Donoghue wrote: > On 11/07/2024 12:41, Depeng Shao wrote: >>>> 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. > > _relaxed is used I'm sure because that's what's always been used and > what downstream does. > > Is there a good reason for it ? None that I can think of. > > Krzysztof is right, there's no good reason to use relaxed() here at all, > you should drop it. > > --- > bod Sure, I will drop the _relaxed. Most io_write don't use _relaxed in the Qualcomm downstream camera driver, but few strict timing required code really have this problem, I will fix them. Thanks for highlight it. Thanks, Depeng
On 11/07/2024 14:00, Bryan O'Donoghue wrote: > On 11/07/2024 12:41, Depeng Shao wrote: >>>> 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. > > _relaxed is used I'm sure because that's what's always been used and > what downstream does. > > Is there a good reason for it ? None that I can think of. > > Krzysztof is right, there's no good reason to use relaxed() here at all, > you should drop it. > In many cases relaxed will be fine, but in few might lead to tricky to debug issues thus people introducing msleep() or other workarounds. Usually init sequences are "sequences" for a reason, but of course here maybe it does not matter. Best regards, Krzysztof
On 7/11/2024 7:12 PM, Krzysztof Kozlowski wrote: > On 11/07/2024 13:08, Depeng Shao wrote: >>> >>>> + >>>> +static void csid_subdev_init(struct csid_device *csid) >>>> +{ >>>> + csid->testgen.modes = csid_testgen_modes; >>>> + csid->testgen.nmodes = CSID_PAYLOAD_MODE_NUM_SUPPORTED_GEN2; >>>> +} >>>> + >>>> +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. > Hi Krzysztof, I'm preparing the next version patches, then I find it is hard to avoid such warning if only apply current patch, since this will be used in the below patch, it will be in structures csid_res_8550 -> sm8550_resources -> camss_dt_match, so I need to add all csid_res_8550, sm8550_resources, camss_dt_match into this patch if I want to avoid the compile warning, then I also need to add compatible info for it to avoid sm8550_resources has unused variable warning, but the sm8550_resources structure also need to add other items to make it complete, then the driver will be incomplete but can be probed with this patch. { .compatible = "qcom,sm8550-camss", .data = &sm8550_resources }, https://lore.kernel.org/all/20240709160656.31146-14-quic_depengs@quicinc.com/ Could you please share some suggestions? > > > Best regards, > Krzysztof > Thanks, Depeng
On 31/07/2024 16:26, Depeng Shao wrote: > I'm preparing the next version patches, then I find it is hard to avoid > such warning if only apply current patch, since this will be used in the > below patch, it will be in structures csid_res_8550 -> sm8550_resources > -> camss_dt_match, so I need to add all csid_res_8550, sm8550_resources, > camss_dt_match into this patch if I want to avoid the compile warning, > then I also need to add compatible info for it to avoid sm8550_resources > has unused variable warning, but the sm8550_resources structure also > need to add other items to make it complete, then the driver will be > incomplete but can be probed with this patch. > > { .compatible = "qcom,sm8550-camss", .data = &sm8550_resources }, > > https://lore.kernel.org/all/20240709160656.31146-14-quic_depengs@quicinc.com/ Couldn't you just add the public structures at the same time they are referenced in &sm8550_resources ? That way your patchset would progressively apply with no warnings. --- bod
Hi Bryan, On 8/1/2024 12:12 AM, Bryan O'Donoghue wrote: > On 31/07/2024 16:26, Depeng Shao wrote: >> I'm preparing the next version patches, then I find it is hard to >> avoid such warning if only apply current patch, since this will be >> used in the below patch, it will be in structures csid_res_8550 -> >> sm8550_resources -> camss_dt_match, so I need to add all >> csid_res_8550, sm8550_resources, camss_dt_match into this patch if I >> want to avoid the compile warning, >> then I also need to add compatible info for it to avoid >> sm8550_resources has unused variable warning, but the sm8550_resources >> structure also need to add other items to make it complete, then the >> driver will be incomplete but can be probed with this patch. >> >> { .compatible = "qcom,sm8550-camss", .data = &sm8550_resources }, >> >> https://lore.kernel.org/all/20240709160656.31146-14-quic_depengs@quicinc.com/ > > Couldn't you just add the public structures at the same time they are > referenced in &sm8550_resources ? > > That way your patchset would progressively apply with no warnings. > Sorry, I don't get it, but in my understanding, but looks like the only way to avoid the compile warning is that adding compatible change in early patch set. I can add the compatible change and structure sm8550_resources in the early patch, but the structure sm8550_resources will just have very few info in this patch. Then fill the other elements in sm8550_resources in the following patches, this can avoid the warning, but the issue is that the sm8550 can be probed once having patch set 1, but the sm8550_resources isn't complete in patch set 1. Could you please common if this is fine? patch set 1 +{ .compatible = "qcom,sm8550-camss", .data = &sm8550_resources }, +static const struct camss_resources sm8550_resources = { + .version = CAMSS_8550, + .pd_name = "top", + .icc_res = icc_res_sm8550, + .icc_path_num = ARRAY_SIZE(icc_res_sm8550), + .link_entities = camss_link_entities +}; patch set2 - Adding csiphy driver and csiphy res to sm8550_resources static const struct camss_resources sm8550_resources = { .version = CAMSS_8550, .pd_name = "top", + .csiphy_res = csiphy_res_8550, .icc_res = icc_res_sm8550, .icc_path_num = ARRAY_SIZE(icc_res_sm8550), + .csiphy_num = ARRAY_SIZE(csiphy_res_8550), .link_entities = camss_link_entities }; patch set 3 - Adding csid driver and csid res to sm8550_resources static const struct camss_resources sm8550_resources = { .version = CAMSS_8550, .pd_name = "top", .csiphy_res = csiphy_res_8550, + .csid_res = csid_res_8550, .icc_res = icc_res_sm8550, .icc_path_num = ARRAY_SIZE(icc_res_sm8550), .csiphy_num = ARRAY_SIZE(csiphy_res_8550), + .csid_num = ARRAY_SIZE(csid_res_8550), .link_entities = camss_link_entities }; > --- > bod Thanks, Depeng
On 01/08/2024 02:53, Depeng Shao wrote: > but the issue is that the sm8550 can be probed once having patch set 1, > but the sm8550_resources isn't complete in patch set 1. Doesn't matter until you _upstream_ a dts that operates on it. Its not a massive deal if you start at patch #1 and and patch #1+n and a there are a few warnings in between as you add stuff in but, for preference every single patch applies and builds warning free. If you - Add bindings - Do driver stuff - Then do dts stuff You don't need to worry about probe() doing mad things. --- bod
On 01/08/2024 11:59, Bryan O'Donoghue wrote:
> for preference every single patch applies and builds warning free.
Oops mistyped
- Every patch must apply cleanly
- You could make an argument for some specific cases that
a patch can generate a warning provided
- By the end of your set everything must be warning free
In this case though, I don't believe you need to make that case since,
the problem you describe about probe() isn't a problem at all as you
have no upstream dts that can drive the probe() at this point.
Just do the dts at the end and no problem.
---
bod
Hi Bryan, On 8/1/2024 7:14 PM, Bryan O'Donoghue wrote: > On 01/08/2024 11:59, Bryan O'Donoghue wrote: >> for preference every single patch applies and builds warning free. > > Oops mistyped > > - Every patch must apply cleanly > - You could make an argument for some specific cases that > a patch can generate a warning provided > - By the end of your set everything must be warning free > > In this case though, I don't believe you need to make that case since, > the problem you describe about probe() isn't a problem at all as you > have no upstream dts that can drive the probe() at this point. > > Just do the dts at the end and no problem. > Thanks for the confirmation, maybe also can add a checking for the res, probe returns fail if the .data->xxx_res is NULL. Thanks, Depeng
> media: qcom: camss: Add CSID Gen3 support for SM8550 > media: qcom: camss: Add support for VFE hardware version Titan 780 Before your post your next version of this series, please make the patch submission titles consistent. e.g. Add CSID 780 support Add VFE 780 support Mixing SoC versions "sm8550" and/or including "Titan" - what's that a reader might ask - should be avoided. No harm in including "Titan" but if you do, include it in both patches and explain that Titan is the codename of the camera block in your SoC. --- bod
V3: - Rebased the change based on below change which will be merged firstly. "Move camss version related defs in to resources" Link: https://lore.kernel.org/all/20240522154659.510-1-quic_grosikop@quicinc.com/ - Rebased the change based on Bryan's csiphy optimization change and add these changes into this series, so that the new csiphy-3ph driver don't need to add duplicate code. This has got Bryan's permission to add his patches into this series. - Refactor some changes based on the comments to move the random code to patches where they are used. - Remove the vfe780 irq function since it isn't doing the actual work. - Add dt-binding for sm8550 camss driver. Link to V2: https://lore.kernel.org/all/20240320141136.26827-1-quic_depengs@quicinc.com/ V2: - Update some commit messages Link to V1: https://lore.kernel.org/all/20240320134227.16587-1-quic_depengs@quicinc.com/ V1: SM8550 is a Qualcomm flagship SoC. This series adds support to bring up the CSIPHY, CSID, VFE/RDI interfaces in SM8550. SM8550 provides - 3 x VFE, 3 RDI per VFE - 2 x VFE Lite, 4 RDI per VFE - 3 x CSID - 2 x CSID Lite - 8 x CSI PHY This series is rebased based on: "Move camss version related defs in to resources" Link: https://lore.kernel.org/all/20240522154659.510-1-quic_grosikop@quicinc.com/ Bryan O'Donoghue (6): media: qcom: camss: csiphy-3ph: Fix trivial indentation fault in defines media: qcom: camss: csiphy-3ph: Remove redundant PHY init sequence control loop media: qcom: camss: csiphy-3ph: Rename struct media: qcom: camss: csiphy: Add an init callback to CSI PHY devices media: qcom: camss: csiphy-3ph: Move CSIPHY variables to data field inside csiphy struct media: qcom: camss: csiphy-3ph: Use an offset variable to find common control regs Depeng Shao (7): dt-bindings: media: camss: Add qcom,sm8550-camss binding media: qcom: camss: csiphy-3ph: Add Gen2 v1.2 two-phase MIPI CSI-2 DPHY init media: qcom: camss: Add CSID Gen3 support for SM8550 media: qcom: camss: Add support for VFE hardware version Titan 780 media: qcom: camss: Add notify interface in camss driver media: qcom: camss: Add sm8550 support media: qcom: camss: Add sm8550 resources .../bindings/media/qcom,sm8550-camss.yaml | 545 ++++++++++++ drivers/media/platform/qcom/camss/Makefile | 2 + .../platform/qcom/camss/camss-csid-gen3.c | 483 +++++++++++ .../platform/qcom/camss/camss-csid-gen3.h | 26 + .../media/platform/qcom/camss/camss-csid.c | 21 + .../media/platform/qcom/camss/camss-csid.h | 11 + .../qcom/camss/camss-csiphy-2ph-1-0.c | 6 + .../qcom/camss/camss-csiphy-3ph-1-0.c | 796 ++++++++++-------- .../media/platform/qcom/camss/camss-csiphy.c | 4 + .../media/platform/qcom/camss/camss-csiphy.h | 2 + .../media/platform/qcom/camss/camss-vfe-780.c | 429 ++++++++++ drivers/media/platform/qcom/camss/camss-vfe.c | 6 + drivers/media/platform/qcom/camss/camss-vfe.h | 2 + drivers/media/platform/qcom/camss/camss.c | 462 ++++++++++ drivers/media/platform/qcom/camss/camss.h | 10 + 15 files changed, 2467 insertions(+), 338 deletions(-) create mode 100644 Documentation/devicetree/bindings/media/qcom,sm8550-camss.yaml create mode 100644 drivers/media/platform/qcom/camss/camss-csid-gen3.c create mode 100644 drivers/media/platform/qcom/camss/camss-csid-gen3.h create mode 100644 drivers/media/platform/qcom/camss/camss-vfe-780.c base-commit: 0c52056d9f77508cb6d4d68d3fc91c6c08ec71af prerequisite-patch-id: 74fabfbf51d650af74a1dc9f7e09fe03a1d85d93 prerequisite-patch-id: 3833c5eed2690679e5eca3551053e08350d2b070 prerequisite-patch-id: 0e433e0c5a9d2402da97c1a01194b1a9f6f7e6cb prerequisite-patch-id: f876f5ed5605fcfebea7f1584888f494bfecd102 prerequisite-patch-id: 8bcb15b208a319a6e88ff4f9e5023097af92cc90 prerequisite-patch-id: dc4b5ddbbedd8773159694b3becad5c7dcbfcf77 prerequisite-patch-id: 75c1b00f4476ee6006041046ad5194b086db3358 prerequisite-patch-id: 0efc004cb8a844291ebe1f16ce192567f44218c7