| Message ID | 20240709160656.31146-5-quic_depengs@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: qcom: camss: Add sm8550 support | expand |
On 7/9/24 19:06, Depeng Shao wrote: > From: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > Add a nop callback to CSIPHY devices. Later patches will enumerate with > enabling code. Please avoid references to "patches" in the commit messages, it'll be changes. I took a look at the next changes, and in my opinion the selected names .init / csiphy_init() / csiphy->res->hw_ops->init(csiphy) are confusing, since there is no intention for real hardware initialization, which is presumed by "hw_ops->init()", I see only routines to get some offsets and populate allocated memory with some values... Not closely a hw_ops. > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > Signed-off-by: Depeng Shao <quic_depengs@quicinc.com> > Reviewed-by: Elliot Berman <quic_eberman@quicinc.com> > --- > drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c | 6 ++++++ > drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c | 6 ++++++ > drivers/media/platform/qcom/camss/camss-csiphy.c | 4 ++++ > drivers/media/platform/qcom/camss/camss-csiphy.h | 1 + > 4 files changed, 17 insertions(+) > > diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c > index cd4a8c369234..9d67e7fa6366 100644 > --- a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c > +++ b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c > @@ -180,6 +180,11 @@ static irqreturn_t csiphy_isr(int irq, void *dev) > return IRQ_HANDLED; > } > > +static int csiphy_init(struct csiphy_device *csiphy) > +{ > + return 0; > +} > + > const struct csiphy_hw_ops csiphy_ops_2ph_1_0 = { > .get_lane_mask = csiphy_get_lane_mask, > .hw_version_read = csiphy_hw_version_read, > @@ -187,4 +192,5 @@ const struct csiphy_hw_ops csiphy_ops_2ph_1_0 = { > .lanes_enable = csiphy_lanes_enable, > .lanes_disable = csiphy_lanes_disable, > .isr = csiphy_isr, > + .init = csiphy_init, > }; > diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c > index bc4834ee2dcc..b60c32a195df 100644 > --- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c > +++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c > @@ -581,6 +581,11 @@ static void csiphy_lanes_disable(struct csiphy_device *csiphy, > CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(6)); > } > > +static int csiphy_init(struct csiphy_device *csiphy) > +{ > + return 0; > +} > + > const struct csiphy_hw_ops csiphy_ops_3ph_1_0 = { > .get_lane_mask = csiphy_get_lane_mask, > .hw_version_read = csiphy_hw_version_read, > @@ -588,4 +593,5 @@ const struct csiphy_hw_ops csiphy_ops_3ph_1_0 = { > .lanes_enable = csiphy_lanes_enable, > .lanes_disable = csiphy_lanes_disable, > .isr = csiphy_isr, > + .init = csiphy_init, > }; > diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c > index 2f7361dfd461..ea5c7078ec8e 100644 > --- a/drivers/media/platform/qcom/camss/camss-csiphy.c > +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c > @@ -576,6 +576,10 @@ int msm_csiphy_subdev_init(struct camss *camss, > csiphy->cfg.combo_mode = 0; > csiphy->res = &res->csiphy; > > + ret = csiphy->res->hw_ops->init(csiphy); Here. > + if (ret) > + return ret; > + > /* Memory */ > > csiphy->base = devm_platform_ioremap_resource_byname(pdev, res->reg[0]); > diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h > index 47f0b6b09eba..bdf9a9c8bacc 100644 > --- a/drivers/media/platform/qcom/camss/camss-csiphy.h > +++ b/drivers/media/platform/qcom/camss/camss-csiphy.h > @@ -71,6 +71,7 @@ struct csiphy_hw_ops { > void (*lanes_disable)(struct csiphy_device *csiphy, > struct csiphy_config *cfg); > irqreturn_t (*isr)(int irq, void *dev); > + int (*init)(struct csiphy_device *csiphy); > }; > > struct csiphy_subdev_resources { -- Best wishes, Vladimir
Hi Bryan, On 8/1/24 11:16, Bryan O'Donoghue wrote: > On 01/08/2024 00:43, Vladimir Zapolskiy wrote: >>> + ret = csiphy->res->hw_ops->init(csiphy); >> >> Here. > > What name would make more sense to you ? according to the implementation the .init() call just fills some data in memory, so I believe this could be handled at build time, if it's done carefully enough... -- Best wishes, Vladimir
Hi Vladimir, On 8/5/2024 5:26 AM, Vladimir Zapolskiy wrote: > Hi Bryan, > > On 8/1/24 11:16, Bryan O'Donoghue wrote: >> On 01/08/2024 00:43, Vladimir Zapolskiy wrote: >>>> + ret = csiphy->res->hw_ops->init(csiphy); >>> >>> Here. >> >> What name would make more sense to you ? > > according to the implementation the .init() call just fills some data in > memory, so I believe this could be handled at build time, if it's done > carefully enough... > This camss-csiphy-3ph-1-0.c is reused by many platforms, the old platforms have same CSI_COMMON_CTR register offset, their offset are 0x800, but some new platforms may have different CSI_COMMON_CTR register offset, for example, the CSI_COMMON_CTR register offset is 0x1000 in sm8550, then we need to add new file to support the new csiphy HW, e.g., camss-csiphy-3ph-2-0.c, so Bryan asked me to develop the CSIPHY driver based on his changes, then we just need few code to enable new CSIPHY. Regarding the hw_ops->init interface, since it fills HW register configurations and HW register offset, then maybe, it also can be called as HW operation. And looks like we can't move it to camss-csiphy.c since it does platform specific operation and it is related to the registers. Please feel free to share other comments if you don't agree with it. Thanks. Thanks, Depeng
On 07/08/2024 14:08, Depeng Shao wrote: > Hi Vladimir, > > On 8/5/2024 5:26 AM, Vladimir Zapolskiy wrote: >> Hi Bryan, >> >> On 8/1/24 11:16, Bryan O'Donoghue wrote: >>> On 01/08/2024 00:43, Vladimir Zapolskiy wrote: >>>>> + ret = csiphy->res->hw_ops->init(csiphy); >>>> >>>> Here. >>> >>> What name would make more sense to you ? >> >> according to the implementation the .init() call just fills some data in >> memory, so I believe this could be handled at build time, if it's done >> carefully enough... >> > > This camss-csiphy-3ph-1-0.c is reused by many platforms, the old > platforms have same CSI_COMMON_CTR register offset, their offset are > 0x800, but some new platforms may have different CSI_COMMON_CTR register > offset, for example, the CSI_COMMON_CTR register offset is 0x1000 in > sm8550, then we need to add new file to support the new csiphy HW, e.g., > camss-csiphy-3ph-2-0.c, so Bryan asked me to develop the CSIPHY driver > based on his changes, then we just need few code to enable new CSIPHY. > > Regarding the hw_ops->init interface, since it fills HW register > configurations and HW register offset, then maybe, it also can be called > as HW operation. > > And looks like we can't move it to camss-csiphy.c since it does platform > specific operation and it is related to the registers. > > Please feel free to share other comments if you don't agree with it. > Thanks. > > > Thanks, > Depeng So, I agree the phy init data could be obtained via resource structs but, rather than add yet more patches to this series, I'd say we can make the move to a separate resource struct pointer at a later date. Lets drop this patch and @Depeng we can then do + regs->offset = 0x800; media: qcom: camss: csiphy-3ph: Use an offset variable to find common control regs As a bonus that's one less patch for this series which @ 13 patches is already large. --- bod
Hi Bryan, On 8/7/2024 10:04 PM, Bryan O'Donoghue wrote: > On 07/08/2024 14:08, Depeng Shao wrote: >> Hi Vladimir, >> >> On 8/5/2024 5:26 AM, Vladimir Zapolskiy wrote: >>> Hi Bryan, >>> >>> On 8/1/24 11:16, Bryan O'Donoghue wrote: >>>> On 01/08/2024 00:43, Vladimir Zapolskiy wrote: >>>>>> + ret = csiphy->res->hw_ops->init(csiphy); >>>>> >>>>> Here. >>>> >>>> What name would make more sense to you ? >>> >>> according to the implementation the .init() call just fills some data in >>> memory, so I believe this could be handled at build time, if it's done >>> carefully enough... >>> >> >> This camss-csiphy-3ph-1-0.c is reused by many platforms, the old >> platforms have same CSI_COMMON_CTR register offset, their offset are >> 0x800, but some new platforms may have different CSI_COMMON_CTR >> register offset, for example, the CSI_COMMON_CTR register offset is >> 0x1000 in sm8550, then we need to add new file to support the new >> csiphy HW, e.g., camss-csiphy-3ph-2-0.c, so Bryan asked me to develop >> the CSIPHY driver based on his changes, then we just need few code to >> enable new CSIPHY. >> >> Regarding the hw_ops->init interface, since it fills HW register >> configurations and HW register offset, then maybe, it also can be >> called as HW operation. >> >> And looks like we can't move it to camss-csiphy.c since it does >> platform specific operation and it is related to the registers. >> >> Please feel free to share other comments if you don't agree with it. >> Thanks. >> >> >> Thanks, >> Depeng > > So, I agree the phy init data could be obtained via resource structs > but, rather than add yet more patches to this series, I'd say we can > make the move to a separate resource struct pointer at a later date. > > Lets drop this patch and @Depeng we can then do > > + regs->offset = 0x800; > > media: qcom: camss: csiphy-3ph: Use an offset variable to find common > control regs > Do you mean only drop "[PATCH 04/13] media: qcom: camss: csiphy: Add an init callback to CSI PHY devices"? [PATCH 05/13] media: qcom: camss: csiphy-3ph: Move CSIPHY variables to data field inside csiphy struct Do you mean this is still needed? Just don't move the code from csiphy_gen2_config_lanes to csiphy_init, right? [PATCH 06/13] media: qcom: camss: csiphy-3ph: Use an offset variable to find common control regs The offset change is also needed, just need to add the offset for different platform in csiphy_gen2_config_lanes . Please correct me if my understanding is wrong. Thanks. > As a bonus that's one less patch for this series which @ 13 patches is > already large. > > --- > bod Thanks, Depeng
On 07/08/2024 16:03, Depeng Shao wrote: > Hi Bryan, > > On 8/7/2024 10:04 PM, Bryan O'Donoghue wrote: >> On 07/08/2024 14:08, Depeng Shao wrote: >>> Hi Vladimir, >>> >>> On 8/5/2024 5:26 AM, Vladimir Zapolskiy wrote: >>>> Hi Bryan, >>>> >>>> On 8/1/24 11:16, Bryan O'Donoghue wrote: >>>>> On 01/08/2024 00:43, Vladimir Zapolskiy wrote: >>>>>>> + ret = csiphy->res->hw_ops->init(csiphy); >>>>>> >>>>>> Here. >>>>> >>>>> What name would make more sense to you ? >>>> >>>> according to the implementation the .init() call just fills some >>>> data in >>>> memory, so I believe this could be handled at build time, if it's done >>>> carefully enough... >>>> >>> >>> This camss-csiphy-3ph-1-0.c is reused by many platforms, the old >>> platforms have same CSI_COMMON_CTR register offset, their offset are >>> 0x800, but some new platforms may have different CSI_COMMON_CTR >>> register offset, for example, the CSI_COMMON_CTR register offset is >>> 0x1000 in sm8550, then we need to add new file to support the new >>> csiphy HW, e.g., camss-csiphy-3ph-2-0.c, so Bryan asked me to develop >>> the CSIPHY driver based on his changes, then we just need few code to >>> enable new CSIPHY. >>> >>> Regarding the hw_ops->init interface, since it fills HW register >>> configurations and HW register offset, then maybe, it also can be >>> called as HW operation. >>> >>> And looks like we can't move it to camss-csiphy.c since it does >>> platform specific operation and it is related to the registers. >>> >>> Please feel free to share other comments if you don't agree with it. >>> Thanks. >>> >>> >>> Thanks, >>> Depeng >> >> So, I agree the phy init data could be obtained via resource structs >> but, rather than add yet more patches to this series, I'd say we can >> make the move to a separate resource struct pointer at a later date. >> >> Lets drop this patch and @Depeng we can then do >> > >> + regs->offset = 0x800; >> >> media: qcom: camss: csiphy-3ph: Use an offset variable to find common >> control regs >> > > > Do you mean only drop "[PATCH 04/13] media: qcom: camss: csiphy: Add an > init callback to CSI PHY devices"? > > > [PATCH 05/13] media: qcom: camss: csiphy-3ph: Move CSIPHY variables to > data field inside csiphy struct > Do you mean this is still needed? Just don't move the code from > csiphy_gen2_config_lanes to csiphy_init, right? > > > [PATCH 06/13] media: qcom: camss: csiphy-3ph: Use an offset variable to > find common control regs > The offset change is also needed, just need to add the offset for > different platform in csiphy_gen2_config_lanes . > > Please correct me if my understanding is wrong. Thanks. Correct. --- bod
Hi Bryan, On 8/7/2024 11:37 PM, Bryan O'Donoghue wrote: > On 07/08/2024 16:03, Depeng Shao wrote: >> Hi Bryan, >> >> On 8/7/2024 10:04 PM, Bryan O'Donoghue wrote: >>> On 07/08/2024 14:08, Depeng Shao wrote: >>>> Hi Vladimir, >>>> >>>> On 8/5/2024 5:26 AM, Vladimir Zapolskiy wrote: >>>>> Hi Bryan, >>>>> >>>>> On 8/1/24 11:16, Bryan O'Donoghue wrote: >>>>>> On 01/08/2024 00:43, Vladimir Zapolskiy wrote: >>>>>>>> + ret = csiphy->res->hw_ops->init(csiphy); >>>>>>> >>>>>>> Here. >>>>>> >>>>>> What name would make more sense to you ? >>>>> >>>>> according to the implementation the .init() call just fills some >>>>> data in >>>>> memory, so I believe this could be handled at build time, if it's done >>>>> carefully enough... >>>>> >>>> >>>> This camss-csiphy-3ph-1-0.c is reused by many platforms, the old >>>> platforms have same CSI_COMMON_CTR register offset, their offset are >>>> 0x800, but some new platforms may have different CSI_COMMON_CTR >>>> register offset, for example, the CSI_COMMON_CTR register offset is >>>> 0x1000 in sm8550, then we need to add new file to support the new >>>> csiphy HW, e.g., camss-csiphy-3ph-2-0.c, so Bryan asked me to >>>> develop the CSIPHY driver based on his changes, then we just need >>>> few code to enable new CSIPHY. >>>> >>>> Regarding the hw_ops->init interface, since it fills HW register >>>> configurations and HW register offset, then maybe, it also can be >>>> called as HW operation. >>>> >>>> And looks like we can't move it to camss-csiphy.c since it does >>>> platform specific operation and it is related to the registers. >>>> >>>> Please feel free to share other comments if you don't agree with it. >>>> Thanks. >>>> >>>> >>>> Thanks, >>>> Depeng >>> >>> So, I agree the phy init data could be obtained via resource structs >>> but, rather than add yet more patches to this series, I'd say we can >>> make the move to a separate resource struct pointer at a later date. >>> >>> Lets drop this patch and @Depeng we can then do >>> >> >>> + regs->offset = 0x800; >>> >>> media: qcom: camss: csiphy-3ph: Use an offset variable to find common >>> control regs >>> >> >> >> Do you mean only drop "[PATCH 04/13] media: qcom: camss: csiphy: Add >> an init callback to CSI PHY devices"? >> >> >> [PATCH 05/13] media: qcom: camss: csiphy-3ph: Move CSIPHY variables to >> data field inside csiphy struct >> Do you mean this is still needed? Just don't move the code from >> csiphy_gen2_config_lanes to csiphy_init, right? >> >> >> [PATCH 06/13] media: qcom: camss: csiphy-3ph: Use an offset variable >> to find common control regs >> The offset change is also needed, just need to add the offset for >> different platform in csiphy_gen2_config_lanes . >> >> Please correct me if my understanding is wrong. Thanks. > > Correct. > I'm updating the code based on above comments, but I meet crash issue if I move the offset assignment to csiphy_gen2_config_lanes, since the csiphy->res->hw_ops->reset(csiphy) is called earlier than csiphy_gen2_config_lanes, so if we don't have the .init interface, we only can move this offset value to `struct csiphy_subdev_resources`, but if we add the offset to `struct csiphy_subdev_resources`, then below two patches are also can be dropped. [PATCH 05/13] media: qcom: camss: csiphy-3ph: Move CSIPHY variables to data field inside csiphy struct [PATCH 06/13] media: qcom: camss: csiphy-3ph: Use an offset variable to find common control regs Could you please comment on if I need to add the CSI_COMMON_CTR offset to res directly? Or add back the .init interface? --- [ 43.162439] Unable to handle kernel NULL pointer dereference at virtual address 000000000000000c [ 43.428307] Call trace: [ 43.430823] csiphy_reset+0x28/0x60 [qcom_camss] [ 43.435572] csiphy_set_power+0x1e8/0x2d4 [qcom_camss] [ 43.440846] pipeline_pm_power_one+0x74/0x10c [videodev] [ 43.446306] pipeline_pm_power+0x44/0xe0 [videodev] [ 43.451313] v4l2_pipeline_pm_get+0x44/0x80 [videodev] [ 43.456588] video_open+0x6c/0xc4 [qcom_camss] [ 43.461158] v4l2_open+0xb8/0x100 [videodev] [ 43.465549] chrdev_open+0x174/0x208 [ 43.469224] do_dentry_open+0x290/0x4b4 [ 43.473164] vfs_open+0x30/0xf0 [ 43.476397] path_openat+0xaec/0xd2c [ 43.480069] do_filp_open+0xb4/0x158 [ 43.483739] do_sys_openat2+0x84/0xe8 [ 43.487500] __arm64_sys_openat+0x70/0x98 [ 43.491619] invoke_syscall+0x40/0xf8 [ 43.495383] el0_svc_common+0xa8/0xd8 [ 43.499143] do_el0_svc+0x1c/0x28 [ 43.502545] el0_svc+0x38/0x68 [ 43.505691] el0t_64_sync_handler+0x90/0xfc [ 43.509989] el0t_64_sync+0x190/0x194 [ 43.513751] Code: 52800028 aa0003f3 52827100 5283e801 (b9400e8a) [ 43.520010] ---[ end trace 0000000000000000 ]--- Segmentation fault Thanks, Depeng
On 08/08/2024 15:02, Depeng Shao wrote: > I'm updating the code based on above comments, but I meet crash issue if > I move the offset assignment to csiphy_gen2_config_lanes, since the > csiphy->res->hw_ops->reset(csiphy) is called earlier than > csiphy_gen2_config_lanes, so if we don't have the .init interface, we > only can move this offset value to `struct csiphy_subdev_resources`, but > if we add the offset to `struct csiphy_subdev_resources`, then below two > patches are also can be dropped. > > > [PATCH 05/13] media: qcom: camss: csiphy-3ph: Move CSIPHY variables to > data field inside csiphy struct > [PATCH 06/13] media: qcom: camss: csiphy-3ph: Use an offset variable to > find common control regs > > > Could you please comment on if I need to add the CSI_COMMON_CTR offset > to res directly? > Or add back the .init interface? Ah, I hadn't recalled why the .init was added -> because sequencing. Lets retain the patch but expand the commit log to explain why the init is being added, instead of jumping through hoops to restructure to get rid of it. --- bod
Hi Bryan, On 8/12/2024 7:32 PM, Bryan O'Donoghue wrote: > > Ah, I hadn't recalled why the .init was added -> because sequencing. > > Lets retain the patch but expand the commit log to explain why the init > is being added, instead of jumping through hoops to restructure to get > rid of it. > Thanks for the confirmation. I will retain the patch and add more commit log. Thanks, Depeng
diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c index cd4a8c369234..9d67e7fa6366 100644 --- a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c +++ b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c @@ -180,6 +180,11 @@ static irqreturn_t csiphy_isr(int irq, void *dev) return IRQ_HANDLED; } +static int csiphy_init(struct csiphy_device *csiphy) +{ + return 0; +} + const struct csiphy_hw_ops csiphy_ops_2ph_1_0 = { .get_lane_mask = csiphy_get_lane_mask, .hw_version_read = csiphy_hw_version_read, @@ -187,4 +192,5 @@ const struct csiphy_hw_ops csiphy_ops_2ph_1_0 = { .lanes_enable = csiphy_lanes_enable, .lanes_disable = csiphy_lanes_disable, .isr = csiphy_isr, + .init = csiphy_init, }; diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c index bc4834ee2dcc..b60c32a195df 100644 --- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c +++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c @@ -581,6 +581,11 @@ static void csiphy_lanes_disable(struct csiphy_device *csiphy, CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(6)); } +static int csiphy_init(struct csiphy_device *csiphy) +{ + return 0; +} + const struct csiphy_hw_ops csiphy_ops_3ph_1_0 = { .get_lane_mask = csiphy_get_lane_mask, .hw_version_read = csiphy_hw_version_read, @@ -588,4 +593,5 @@ const struct csiphy_hw_ops csiphy_ops_3ph_1_0 = { .lanes_enable = csiphy_lanes_enable, .lanes_disable = csiphy_lanes_disable, .isr = csiphy_isr, + .init = csiphy_init, }; diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c index 2f7361dfd461..ea5c7078ec8e 100644 --- a/drivers/media/platform/qcom/camss/camss-csiphy.c +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c @@ -576,6 +576,10 @@ int msm_csiphy_subdev_init(struct camss *camss, csiphy->cfg.combo_mode = 0; csiphy->res = &res->csiphy; + ret = csiphy->res->hw_ops->init(csiphy); + if (ret) + return ret; + /* Memory */ csiphy->base = devm_platform_ioremap_resource_byname(pdev, res->reg[0]); diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h index 47f0b6b09eba..bdf9a9c8bacc 100644 --- a/drivers/media/platform/qcom/camss/camss-csiphy.h +++ b/drivers/media/platform/qcom/camss/camss-csiphy.h @@ -71,6 +71,7 @@ struct csiphy_hw_ops { void (*lanes_disable)(struct csiphy_device *csiphy, struct csiphy_config *cfg); irqreturn_t (*isr)(int irq, void *dev); + int (*init)(struct csiphy_device *csiphy); }; struct csiphy_subdev_resources {