| Message ID | 20241212-fd-dp-audio-fixup-v3-4-0b1c65e7dba3@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm/dp: perform misc cleanups | expand |
On 12/11/2024 3:41 PM, Dmitry Baryshkov wrote: > Having I/O regions inside a msm_dp_catalog_private() results in extra > layers of one-line wrappers for accessing the data. Move I/O region base > and size to the globally visible struct msm_dp_catalog. > > Reviewed-by: Stephen Boyd <swboyd@chromium.org> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/dp/dp_catalog.c | 457 +++++++++++++++--------------------- > drivers/gpu/drm/msm/dp/dp_catalog.h | 12 + > 2 files changed, 197 insertions(+), 272 deletions(-) > Fundamentally, the whole point of catalog was that it needs to be the only place where we want to access the registers. Thats how this really started. This pre-dates my time with the DP driver but as I understand thats what it was for. Basically separating out the logical abstraction vs actual register writes . If there are hardware sequence differences within the controller reset OR any other register offsets which moved around, catalog should have been able to absorb it without that spilling over to all the layers. So for example, if we call dp_ctrl_reset() --> ctrl->catalog->reset_ctrl() Then the reset_ctrl op of the catalog should manage any controller version differences within the reset sequence. We do not use or have catalog ops today so it looks redundant as we just call the dp_catalog APIs directly but that was really the intention. Another reason which was behind this split but not applicable to current upstream driver is that the AUX is part of the PHY driver in upstream but in downstream, that remains a part of catalog and as we know the AUX component keeps changing with chipsets especially the settings. That was the reason of keeping catalog separate and the only place which should deal with registers and not the entire DP driver. The second point seems not applicable to this driver but first point still is. I do admit there is re-direction like ctrl->catalog instead of just writing it within dp_ctrl itself but the redirection was only because ctrl layers were not really meant to deal with the register programming. So for example, now with patch 7 of this series every register being written to i exposed in dp_ctrl.c and likewise for other files. That seems unnecessary. Because if we do end up with some variants which need separate registers written, then we will now have to end up touching every file as opposed to only touching dp_catalog. > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c > index 0357dec1acd5773f25707e7ebdfca4b1d2b1bb4e..cdb8685924a06e4fc79d70586630ccb9a16a676d 100644 > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c > @@ -63,155 +63,128 @@ > #define DP_DEFAULT_P0_OFFSET 0x1000 > #define DP_DEFAULT_P0_SIZE 0x0400 > > -struct dss_io_region { > - size_t len; > - void __iomem *base; > -}; > - > -struct dss_io_data { > - struct dss_io_region ahb; > - struct dss_io_region aux; > - struct dss_io_region link; > - struct dss_io_region p0; > -}; > - > struct msm_dp_catalog_private { > struct device *dev; > struct drm_device *drm_dev; > - struct dss_io_data io; > u32 (*audio_map)[DP_AUDIO_SDP_HEADER_MAX]; > struct msm_dp_catalog msm_dp_catalog; > }; > > void msm_dp_catalog_snapshot(struct msm_dp_catalog *msm_dp_catalog, struct msm_disp_state *disp_state) > { > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > - struct dss_io_data *dss = &catalog->io; > - > - msm_disp_snapshot_add_block(disp_state, dss->ahb.len, dss->ahb.base, "dp_ahb"); > - msm_disp_snapshot_add_block(disp_state, dss->aux.len, dss->aux.base, "dp_aux"); > - msm_disp_snapshot_add_block(disp_state, dss->link.len, dss->link.base, "dp_link"); > - msm_disp_snapshot_add_block(disp_state, dss->p0.len, dss->p0.base, "dp_p0"); > + msm_disp_snapshot_add_block(disp_state, > + msm_dp_catalog->ahb_len, msm_dp_catalog->ahb_base, "dp_ahb"); > + msm_disp_snapshot_add_block(disp_state, > + msm_dp_catalog->aux_len, msm_dp_catalog->aux_base, "dp_aux"); > + msm_disp_snapshot_add_block(disp_state, > + msm_dp_catalog->link_len, msm_dp_catalog->link_base, "dp_link"); > + msm_disp_snapshot_add_block(disp_state, > + msm_dp_catalog->p0_len, msm_dp_catalog->p0_base, "dp_p0"); > } > > -static inline u32 msm_dp_read_aux(struct msm_dp_catalog_private *catalog, u32 offset) > +static inline u32 msm_dp_read_aux(struct msm_dp_catalog *msm_dp_catalog, u32 offset) > { > - return readl_relaxed(catalog->io.aux.base + offset); > + return readl_relaxed(msm_dp_catalog->aux_base + offset); > } > > -static inline void msm_dp_write_aux(struct msm_dp_catalog_private *catalog, > +static inline void msm_dp_write_aux(struct msm_dp_catalog *msm_dp_catalog, > u32 offset, u32 data) > { > /* > * To make sure aux reg writes happens before any other operation, > * this function uses writel() instread of writel_relaxed() > */ > - writel(data, catalog->io.aux.base + offset); > + writel(data, msm_dp_catalog->aux_base + offset); > } > > -static inline u32 msm_dp_read_ahb(const struct msm_dp_catalog_private *catalog, u32 offset) > +static inline u32 msm_dp_read_ahb(const struct msm_dp_catalog *msm_dp_catalog, u32 offset) > { > - return readl_relaxed(catalog->io.ahb.base + offset); > + return readl_relaxed(msm_dp_catalog->ahb_base + offset); > } > > -static inline void msm_dp_write_ahb(struct msm_dp_catalog_private *catalog, > +static inline void msm_dp_write_ahb(struct msm_dp_catalog *msm_dp_catalog, > u32 offset, u32 data) > { > /* > * To make sure phy reg writes happens before any other operation, > * this function uses writel() instread of writel_relaxed() > */ > - writel(data, catalog->io.ahb.base + offset); > + writel(data, msm_dp_catalog->ahb_base + offset); > } > > -static inline void msm_dp_write_p0(struct msm_dp_catalog_private *catalog, > +static inline void msm_dp_write_p0(struct msm_dp_catalog *msm_dp_catalog, > u32 offset, u32 data) > { > /* > * To make sure interface reg writes happens before any other operation, > * this function uses writel() instread of writel_relaxed() > */ > - writel(data, catalog->io.p0.base + offset); > + writel(data, msm_dp_catalog->p0_base + offset); > } > > -static inline u32 msm_dp_read_p0(struct msm_dp_catalog_private *catalog, > +static inline u32 msm_dp_read_p0(struct msm_dp_catalog *msm_dp_catalog, > u32 offset) > { > /* > * To make sure interface reg writes happens before any other operation, > * this function uses writel() instread of writel_relaxed() > */ > - return readl_relaxed(catalog->io.p0.base + offset); > + return readl_relaxed(msm_dp_catalog->p0_base + offset); > } > > -static inline u32 msm_dp_read_link(struct msm_dp_catalog_private *catalog, u32 offset) > +static inline u32 msm_dp_read_link(struct msm_dp_catalog *msm_dp_catalog, u32 offset) > { > - return readl_relaxed(catalog->io.link.base + offset); > + return readl_relaxed(msm_dp_catalog->link_base + offset); > } > > -static inline void msm_dp_write_link(struct msm_dp_catalog_private *catalog, > +static inline void msm_dp_write_link(struct msm_dp_catalog *msm_dp_catalog, > u32 offset, u32 data) > { > /* > * To make sure link reg writes happens before any other operation, > * this function uses writel() instread of writel_relaxed() > */ > - writel(data, catalog->io.link.base + offset); > + writel(data, msm_dp_catalog->link_base + offset); > } > > /* aux related catalog functions */ > u32 msm_dp_catalog_aux_read_data(struct msm_dp_catalog *msm_dp_catalog) > { > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > - > - return msm_dp_read_aux(catalog, REG_DP_AUX_DATA); > + return msm_dp_read_aux(msm_dp_catalog, REG_DP_AUX_DATA); > } > > int msm_dp_catalog_aux_write_data(struct msm_dp_catalog *msm_dp_catalog, u32 data) > { > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > - > - msm_dp_write_aux(catalog, REG_DP_AUX_DATA, data); > + msm_dp_write_aux(msm_dp_catalog, REG_DP_AUX_DATA, data); > return 0; > } > > int msm_dp_catalog_aux_write_trans(struct msm_dp_catalog *msm_dp_catalog, u32 data) > { > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > - > - msm_dp_write_aux(catalog, REG_DP_AUX_TRANS_CTRL, data); > + msm_dp_write_aux(msm_dp_catalog, REG_DP_AUX_TRANS_CTRL, data); > return 0; > } > > int msm_dp_catalog_aux_clear_trans(struct msm_dp_catalog *msm_dp_catalog, bool read) > { > u32 data; > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > > if (read) { > - data = msm_dp_read_aux(catalog, REG_DP_AUX_TRANS_CTRL); > + data = msm_dp_read_aux(msm_dp_catalog, REG_DP_AUX_TRANS_CTRL); > data &= ~DP_AUX_TRANS_CTRL_GO; > - msm_dp_write_aux(catalog, REG_DP_AUX_TRANS_CTRL, data); > + msm_dp_write_aux(msm_dp_catalog, REG_DP_AUX_TRANS_CTRL, data); > } else { > - msm_dp_write_aux(catalog, REG_DP_AUX_TRANS_CTRL, 0); > + msm_dp_write_aux(msm_dp_catalog, REG_DP_AUX_TRANS_CTRL, 0); > } > return 0; > } > > int msm_dp_catalog_aux_clear_hw_interrupts(struct msm_dp_catalog *msm_dp_catalog) > { > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > - > - msm_dp_read_aux(catalog, REG_DP_PHY_AUX_INTERRUPT_STATUS); > - msm_dp_write_aux(catalog, REG_DP_PHY_AUX_INTERRUPT_CLEAR, 0x1f); > - msm_dp_write_aux(catalog, REG_DP_PHY_AUX_INTERRUPT_CLEAR, 0x9f); > - msm_dp_write_aux(catalog, REG_DP_PHY_AUX_INTERRUPT_CLEAR, 0); > + msm_dp_read_aux(msm_dp_catalog, REG_DP_PHY_AUX_INTERRUPT_STATUS); > + msm_dp_write_aux(msm_dp_catalog, REG_DP_PHY_AUX_INTERRUPT_CLEAR, 0x1f); > + msm_dp_write_aux(msm_dp_catalog, REG_DP_PHY_AUX_INTERRUPT_CLEAR, 0x9f); > + msm_dp_write_aux(msm_dp_catalog, REG_DP_PHY_AUX_INTERRUPT_CLEAR, 0); > return 0; > } > > @@ -230,47 +203,41 @@ int msm_dp_catalog_aux_clear_hw_interrupts(struct msm_dp_catalog *msm_dp_catalog > void msm_dp_catalog_aux_reset(struct msm_dp_catalog *msm_dp_catalog) > { > u32 aux_ctrl; > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > > - aux_ctrl = msm_dp_read_aux(catalog, REG_DP_AUX_CTRL); > + aux_ctrl = msm_dp_read_aux(msm_dp_catalog, REG_DP_AUX_CTRL); > > aux_ctrl |= DP_AUX_CTRL_RESET; > - msm_dp_write_aux(catalog, REG_DP_AUX_CTRL, aux_ctrl); > + msm_dp_write_aux(msm_dp_catalog, REG_DP_AUX_CTRL, aux_ctrl); > usleep_range(1000, 1100); /* h/w recommended delay */ > > aux_ctrl &= ~DP_AUX_CTRL_RESET; > - msm_dp_write_aux(catalog, REG_DP_AUX_CTRL, aux_ctrl); > + msm_dp_write_aux(msm_dp_catalog, REG_DP_AUX_CTRL, aux_ctrl); > } > > void msm_dp_catalog_aux_enable(struct msm_dp_catalog *msm_dp_catalog, bool enable) > { > u32 aux_ctrl; > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > > - aux_ctrl = msm_dp_read_aux(catalog, REG_DP_AUX_CTRL); > + aux_ctrl = msm_dp_read_aux(msm_dp_catalog, REG_DP_AUX_CTRL); > > if (enable) { > - msm_dp_write_aux(catalog, REG_DP_TIMEOUT_COUNT, 0xffff); > - msm_dp_write_aux(catalog, REG_DP_AUX_LIMITS, 0xffff); > + msm_dp_write_aux(msm_dp_catalog, REG_DP_TIMEOUT_COUNT, 0xffff); > + msm_dp_write_aux(msm_dp_catalog, REG_DP_AUX_LIMITS, 0xffff); > aux_ctrl |= DP_AUX_CTRL_ENABLE; > } else { > aux_ctrl &= ~DP_AUX_CTRL_ENABLE; > } > > - msm_dp_write_aux(catalog, REG_DP_AUX_CTRL, aux_ctrl); > + msm_dp_write_aux(msm_dp_catalog, REG_DP_AUX_CTRL, aux_ctrl); > } > > int msm_dp_catalog_aux_wait_for_hpd_connect_state(struct msm_dp_catalog *msm_dp_catalog, > unsigned long wait_us) > { > u32 state; > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > > /* poll for hpd connected status every 2ms and timeout after wait_us */ > - return readl_poll_timeout(catalog->io.aux.base + > + return readl_poll_timeout(msm_dp_catalog->aux_base + > REG_DP_DP_HPD_INT_STATUS, > state, state & DP_DP_HPD_STATE_STATUS_CONNECTED, > min(wait_us, 2000), wait_us); > @@ -278,15 +245,13 @@ int msm_dp_catalog_aux_wait_for_hpd_connect_state(struct msm_dp_catalog *msm_dp_ > > u32 msm_dp_catalog_aux_get_irq(struct msm_dp_catalog *msm_dp_catalog) > { > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > u32 intr, intr_ack; > > - intr = msm_dp_read_ahb(catalog, REG_DP_INTR_STATUS); > + intr = msm_dp_read_ahb(msm_dp_catalog, REG_DP_INTR_STATUS); > intr &= ~DP_INTERRUPT_STATUS1_MASK; > intr_ack = (intr & DP_INTERRUPT_STATUS1) > << DP_INTERRUPT_STATUS_ACK_SHIFT; > - msm_dp_write_ahb(catalog, REG_DP_INTR_STATUS, intr_ack | > + msm_dp_write_ahb(msm_dp_catalog, REG_DP_INTR_STATUS, intr_ack | > DP_INTERRUPT_STATUS1_MASK); > > return intr; > @@ -298,20 +263,14 @@ void msm_dp_catalog_ctrl_update_transfer_unit(struct msm_dp_catalog *msm_dp_cata > u32 msm_dp_tu, u32 valid_boundary, > u32 valid_boundary2) > { > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > - > - msm_dp_write_link(catalog, REG_DP_VALID_BOUNDARY, valid_boundary); > - msm_dp_write_link(catalog, REG_DP_TU, msm_dp_tu); > - msm_dp_write_link(catalog, REG_DP_VALID_BOUNDARY_2, valid_boundary2); > + msm_dp_write_link(msm_dp_catalog, REG_DP_VALID_BOUNDARY, valid_boundary); > + msm_dp_write_link(msm_dp_catalog, REG_DP_TU, msm_dp_tu); > + msm_dp_write_link(msm_dp_catalog, REG_DP_VALID_BOUNDARY_2, valid_boundary2); > } > > void msm_dp_catalog_ctrl_state_ctrl(struct msm_dp_catalog *msm_dp_catalog, u32 state) > { > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > - > - msm_dp_write_link(catalog, REG_DP_STATE_CTRL, state); > + msm_dp_write_link(msm_dp_catalog, REG_DP_STATE_CTRL, state); > } > > void msm_dp_catalog_ctrl_config_ctrl(struct msm_dp_catalog *msm_dp_catalog, u32 cfg) > @@ -321,13 +280,11 @@ void msm_dp_catalog_ctrl_config_ctrl(struct msm_dp_catalog *msm_dp_catalog, u32 > > drm_dbg_dp(catalog->drm_dev, "DP_CONFIGURATION_CTRL=0x%x\n", cfg); > > - msm_dp_write_link(catalog, REG_DP_CONFIGURATION_CTRL, cfg); > + msm_dp_write_link(msm_dp_catalog, REG_DP_CONFIGURATION_CTRL, cfg); > } > > void msm_dp_catalog_ctrl_lane_mapping(struct msm_dp_catalog *msm_dp_catalog) > { > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > u32 ln_0 = 0, ln_1 = 1, ln_2 = 2, ln_3 = 3; /* One-to-One mapping */ > u32 ln_mapping; > > @@ -336,7 +293,7 @@ void msm_dp_catalog_ctrl_lane_mapping(struct msm_dp_catalog *msm_dp_catalog) > ln_mapping |= ln_2 << LANE2_MAPPING_SHIFT; > ln_mapping |= ln_3 << LANE3_MAPPING_SHIFT; > > - msm_dp_write_link(catalog, REG_DP_LOGICAL2PHYSICAL_LANE_MAPPING, > + msm_dp_write_link(msm_dp_catalog, REG_DP_LOGICAL2PHYSICAL_LANE_MAPPING, > ln_mapping); > } > > @@ -344,17 +301,15 @@ void msm_dp_catalog_ctrl_psr_mainlink_enable(struct msm_dp_catalog *msm_dp_catal > bool enable) > { > u32 val; > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > > - val = msm_dp_read_link(catalog, REG_DP_MAINLINK_CTRL); > + val = msm_dp_read_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL); > > if (enable) > val |= DP_MAINLINK_CTRL_ENABLE; > else > val &= ~DP_MAINLINK_CTRL_ENABLE; > > - msm_dp_write_link(catalog, REG_DP_MAINLINK_CTRL, val); > + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL, val); > } > > void msm_dp_catalog_ctrl_mainlink_ctrl(struct msm_dp_catalog *msm_dp_catalog, > @@ -370,25 +325,25 @@ void msm_dp_catalog_ctrl_mainlink_ctrl(struct msm_dp_catalog *msm_dp_catalog, > * To make sure link reg writes happens before other operation, > * msm_dp_write_link() function uses writel() > */ > - mainlink_ctrl = msm_dp_read_link(catalog, REG_DP_MAINLINK_CTRL); > + mainlink_ctrl = msm_dp_read_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL); > > mainlink_ctrl &= ~(DP_MAINLINK_CTRL_RESET | > DP_MAINLINK_CTRL_ENABLE); > - msm_dp_write_link(catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); > + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); > > mainlink_ctrl |= DP_MAINLINK_CTRL_RESET; > - msm_dp_write_link(catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); > + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); > > mainlink_ctrl &= ~DP_MAINLINK_CTRL_RESET; > - msm_dp_write_link(catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); > + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); > > mainlink_ctrl |= (DP_MAINLINK_CTRL_ENABLE | > DP_MAINLINK_FB_BOUNDARY_SEL); > - msm_dp_write_link(catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); > + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); > } else { > - mainlink_ctrl = msm_dp_read_link(catalog, REG_DP_MAINLINK_CTRL); > + mainlink_ctrl = msm_dp_read_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL); > mainlink_ctrl &= ~DP_MAINLINK_CTRL_ENABLE; > - msm_dp_write_link(catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); > + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); > } > } > > @@ -400,7 +355,7 @@ void msm_dp_catalog_ctrl_config_misc(struct msm_dp_catalog *msm_dp_catalog, > struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > struct msm_dp_catalog_private, msm_dp_catalog); > > - misc_val = msm_dp_read_link(catalog, REG_DP_MISC1_MISC0); > + misc_val = msm_dp_read_link(msm_dp_catalog, REG_DP_MISC1_MISC0); > > /* clear bpp bits */ > misc_val &= ~(0x07 << DP_MISC0_TEST_BITS_DEPTH_SHIFT); > @@ -410,16 +365,14 @@ void msm_dp_catalog_ctrl_config_misc(struct msm_dp_catalog *msm_dp_catalog, > misc_val |= DP_MISC0_SYNCHRONOUS_CLK; > > drm_dbg_dp(catalog->drm_dev, "misc settings = 0x%x\n", misc_val); > - msm_dp_write_link(catalog, REG_DP_MISC1_MISC0, misc_val); > + msm_dp_write_link(msm_dp_catalog, REG_DP_MISC1_MISC0, misc_val); > } > > void msm_dp_catalog_setup_peripheral_flush(struct msm_dp_catalog *msm_dp_catalog) > { > u32 mainlink_ctrl, hw_revision; > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > > - mainlink_ctrl = msm_dp_read_link(catalog, REG_DP_MAINLINK_CTRL); > + mainlink_ctrl = msm_dp_read_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL); > > hw_revision = msm_dp_catalog_hw_revision(msm_dp_catalog); > if (hw_revision >= DP_HW_VERSION_1_2) > @@ -427,7 +380,7 @@ void msm_dp_catalog_setup_peripheral_flush(struct msm_dp_catalog *msm_dp_catalog > else > mainlink_ctrl |= DP_MAINLINK_FLUSH_MODE_UPDATE_SDP; > > - msm_dp_write_link(catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); > + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); > } > > void msm_dp_catalog_ctrl_config_msa(struct msm_dp_catalog *msm_dp_catalog, > @@ -485,9 +438,9 @@ void msm_dp_catalog_ctrl_config_msa(struct msm_dp_catalog *msm_dp_catalog, > nvid *= 3; > > drm_dbg_dp(catalog->drm_dev, "mvid=0x%x, nvid=0x%x\n", mvid, nvid); > - msm_dp_write_link(catalog, REG_DP_SOFTWARE_MVID, mvid); > - msm_dp_write_link(catalog, REG_DP_SOFTWARE_NVID, nvid); > - msm_dp_write_p0(catalog, MMSS_DP_DSC_DTO, 0x0); > + msm_dp_write_link(msm_dp_catalog, REG_DP_SOFTWARE_MVID, mvid); > + msm_dp_write_link(msm_dp_catalog, REG_DP_SOFTWARE_NVID, nvid); > + msm_dp_write_p0(msm_dp_catalog, MMSS_DP_DSC_DTO, 0x0); > } > > int msm_dp_catalog_ctrl_set_pattern_state_bit(struct msm_dp_catalog *msm_dp_catalog, > @@ -505,7 +458,7 @@ int msm_dp_catalog_ctrl_set_pattern_state_bit(struct msm_dp_catalog *msm_dp_cata > bit = BIT(state_bit - 1) << DP_MAINLINK_READY_LINK_TRAINING_SHIFT; > > /* Poll for mainlink ready status */ > - ret = readx_poll_timeout(readl, catalog->io.link.base + > + ret = readx_poll_timeout(readl, msm_dp_catalog->link_base + > REG_DP_MAINLINK_READY, > data, data & bit, > POLLING_SLEEP_US, POLLING_TIMEOUT_US); > @@ -526,10 +479,7 @@ int msm_dp_catalog_ctrl_set_pattern_state_bit(struct msm_dp_catalog *msm_dp_cata > */ > u32 msm_dp_catalog_hw_revision(const struct msm_dp_catalog *msm_dp_catalog) > { > - const struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > - > - return msm_dp_read_ahb(catalog, REG_DP_HW_VERSION); > + return msm_dp_read_ahb(msm_dp_catalog, REG_DP_HW_VERSION); > } > > /** > @@ -547,28 +497,24 @@ u32 msm_dp_catalog_hw_revision(const struct msm_dp_catalog *msm_dp_catalog) > void msm_dp_catalog_ctrl_reset(struct msm_dp_catalog *msm_dp_catalog) > { > u32 sw_reset; > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > > - sw_reset = msm_dp_read_ahb(catalog, REG_DP_SW_RESET); > + sw_reset = msm_dp_read_ahb(msm_dp_catalog, REG_DP_SW_RESET); > > sw_reset |= DP_SW_RESET; > - msm_dp_write_ahb(catalog, REG_DP_SW_RESET, sw_reset); > + msm_dp_write_ahb(msm_dp_catalog, REG_DP_SW_RESET, sw_reset); > usleep_range(1000, 1100); /* h/w recommended delay */ > > sw_reset &= ~DP_SW_RESET; > - msm_dp_write_ahb(catalog, REG_DP_SW_RESET, sw_reset); > + msm_dp_write_ahb(msm_dp_catalog, REG_DP_SW_RESET, sw_reset); > } > > bool msm_dp_catalog_ctrl_mainlink_ready(struct msm_dp_catalog *msm_dp_catalog) > { > u32 data; > int ret; > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > > /* Poll for mainlink ready status */ > - ret = readl_poll_timeout(catalog->io.link.base + > + ret = readl_poll_timeout(msm_dp_catalog->link_base + > REG_DP_MAINLINK_READY, > data, data & DP_MAINLINK_READY_FOR_VIDEO, > POLLING_SLEEP_US, POLLING_TIMEOUT_US); > @@ -583,17 +529,14 @@ bool msm_dp_catalog_ctrl_mainlink_ready(struct msm_dp_catalog *msm_dp_catalog) > void msm_dp_catalog_ctrl_enable_irq(struct msm_dp_catalog *msm_dp_catalog, > bool enable) > { > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > - > if (enable) { > - msm_dp_write_ahb(catalog, REG_DP_INTR_STATUS, > + msm_dp_write_ahb(msm_dp_catalog, REG_DP_INTR_STATUS, > DP_INTERRUPT_STATUS1_MASK); > - msm_dp_write_ahb(catalog, REG_DP_INTR_STATUS2, > + msm_dp_write_ahb(msm_dp_catalog, REG_DP_INTR_STATUS2, > DP_INTERRUPT_STATUS2_MASK); > } else { > - msm_dp_write_ahb(catalog, REG_DP_INTR_STATUS, 0x00); > - msm_dp_write_ahb(catalog, REG_DP_INTR_STATUS2, 0x00); > + msm_dp_write_ahb(msm_dp_catalog, REG_DP_INTR_STATUS, 0x00); > + msm_dp_write_ahb(msm_dp_catalog, REG_DP_INTR_STATUS2, 0x00); > } > } > > @@ -603,73 +546,63 @@ void msm_dp_catalog_hpd_config_intr(struct msm_dp_catalog *msm_dp_catalog, > struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > struct msm_dp_catalog_private, msm_dp_catalog); > > - u32 config = msm_dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK); > + u32 config = msm_dp_read_aux(msm_dp_catalog, REG_DP_DP_HPD_INT_MASK); > > config = (en ? config | intr_mask : config & ~intr_mask); > > drm_dbg_dp(catalog->drm_dev, "intr_mask=%#x config=%#x\n", > intr_mask, config); > - msm_dp_write_aux(catalog, REG_DP_DP_HPD_INT_MASK, > + msm_dp_write_aux(msm_dp_catalog, REG_DP_DP_HPD_INT_MASK, > config & DP_DP_HPD_INT_MASK); > } > > void msm_dp_catalog_ctrl_hpd_enable(struct msm_dp_catalog *msm_dp_catalog) > { > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > - > - u32 reftimer = msm_dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER); > + u32 reftimer = msm_dp_read_aux(msm_dp_catalog, REG_DP_DP_HPD_REFTIMER); > > /* Configure REFTIMER and enable it */ > reftimer |= DP_DP_HPD_REFTIMER_ENABLE; > - msm_dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer); > + msm_dp_write_aux(msm_dp_catalog, REG_DP_DP_HPD_REFTIMER, reftimer); > > /* Enable HPD */ > - msm_dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, DP_DP_HPD_CTRL_HPD_EN); > + msm_dp_write_aux(msm_dp_catalog, REG_DP_DP_HPD_CTRL, DP_DP_HPD_CTRL_HPD_EN); > } > > void msm_dp_catalog_ctrl_hpd_disable(struct msm_dp_catalog *msm_dp_catalog) > { > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > - > - u32 reftimer = msm_dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER); > + u32 reftimer = msm_dp_read_aux(msm_dp_catalog, REG_DP_DP_HPD_REFTIMER); > > reftimer &= ~DP_DP_HPD_REFTIMER_ENABLE; > - msm_dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer); > + msm_dp_write_aux(msm_dp_catalog, REG_DP_DP_HPD_REFTIMER, reftimer); > > - msm_dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, 0); > + msm_dp_write_aux(msm_dp_catalog, REG_DP_DP_HPD_CTRL, 0); > } > > -static void msm_dp_catalog_enable_sdp(struct msm_dp_catalog_private *catalog) > +static void msm_dp_catalog_enable_sdp(struct msm_dp_catalog *msm_dp_catalog) > { > /* trigger sdp */ > - msm_dp_write_link(catalog, MMSS_DP_SDP_CFG3, UPDATE_SDP); > - msm_dp_write_link(catalog, MMSS_DP_SDP_CFG3, 0x0); > + msm_dp_write_link(msm_dp_catalog, MMSS_DP_SDP_CFG3, UPDATE_SDP); > + msm_dp_write_link(msm_dp_catalog, MMSS_DP_SDP_CFG3, 0x0); > } > > void msm_dp_catalog_ctrl_config_psr(struct msm_dp_catalog *msm_dp_catalog) > { > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > u32 config; > > /* enable PSR1 function */ > - config = msm_dp_read_link(catalog, REG_PSR_CONFIG); > + config = msm_dp_read_link(msm_dp_catalog, REG_PSR_CONFIG); > config |= PSR1_SUPPORTED; > - msm_dp_write_link(catalog, REG_PSR_CONFIG, config); > + msm_dp_write_link(msm_dp_catalog, REG_PSR_CONFIG, config); > > - msm_dp_write_ahb(catalog, REG_DP_INTR_MASK4, DP_INTERRUPT_MASK4); > - msm_dp_catalog_enable_sdp(catalog); > + msm_dp_write_ahb(msm_dp_catalog, REG_DP_INTR_MASK4, DP_INTERRUPT_MASK4); > + msm_dp_catalog_enable_sdp(msm_dp_catalog); > } > > void msm_dp_catalog_ctrl_set_psr(struct msm_dp_catalog *msm_dp_catalog, bool enter) > { > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > u32 cmd; > > - cmd = msm_dp_read_link(catalog, REG_PSR_CMD); > + cmd = msm_dp_read_link(msm_dp_catalog, REG_PSR_CMD); > > cmd &= ~(PSR_ENTER | PSR_EXIT); > > @@ -678,8 +611,8 @@ void msm_dp_catalog_ctrl_set_psr(struct msm_dp_catalog *msm_dp_catalog, bool ent > else > cmd |= PSR_EXIT; > > - msm_dp_catalog_enable_sdp(catalog); > - msm_dp_write_link(catalog, REG_PSR_CMD, cmd); > + msm_dp_catalog_enable_sdp(msm_dp_catalog); > + msm_dp_write_link(msm_dp_catalog, REG_PSR_CMD, cmd); > } > > u32 msm_dp_catalog_link_is_connected(struct msm_dp_catalog *msm_dp_catalog) > @@ -688,7 +621,7 @@ u32 msm_dp_catalog_link_is_connected(struct msm_dp_catalog *msm_dp_catalog) > struct msm_dp_catalog_private, msm_dp_catalog); > u32 status; > > - status = msm_dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS); > + status = msm_dp_read_aux(msm_dp_catalog, REG_DP_DP_HPD_INT_STATUS); > drm_dbg_dp(catalog->drm_dev, "aux status: %#x\n", status); > status >>= DP_DP_HPD_STATE_STATUS_BITS_SHIFT; > status &= DP_DP_HPD_STATE_STATUS_BITS_MASK; > @@ -698,14 +631,12 @@ u32 msm_dp_catalog_link_is_connected(struct msm_dp_catalog *msm_dp_catalog) > > u32 msm_dp_catalog_hpd_get_intr_status(struct msm_dp_catalog *msm_dp_catalog) > { > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > int isr, mask; > > - isr = msm_dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS); > - msm_dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK, > + isr = msm_dp_read_aux(msm_dp_catalog, REG_DP_DP_HPD_INT_STATUS); > + msm_dp_write_aux(msm_dp_catalog, REG_DP_DP_HPD_INT_ACK, > (isr & DP_DP_HPD_INT_MASK)); > - mask = msm_dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK); > + mask = msm_dp_read_aux(msm_dp_catalog, REG_DP_DP_HPD_INT_MASK); > > /* > * We only want to return interrupts that are unmasked to the caller. > @@ -719,29 +650,25 @@ u32 msm_dp_catalog_hpd_get_intr_status(struct msm_dp_catalog *msm_dp_catalog) > > u32 msm_dp_catalog_ctrl_read_psr_interrupt_status(struct msm_dp_catalog *msm_dp_catalog) > { > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > u32 intr, intr_ack; > > - intr = msm_dp_read_ahb(catalog, REG_DP_INTR_STATUS4); > + intr = msm_dp_read_ahb(msm_dp_catalog, REG_DP_INTR_STATUS4); > intr_ack = (intr & DP_INTERRUPT_STATUS4) > << DP_INTERRUPT_STATUS_ACK_SHIFT; > - msm_dp_write_ahb(catalog, REG_DP_INTR_STATUS4, intr_ack); > + msm_dp_write_ahb(msm_dp_catalog, REG_DP_INTR_STATUS4, intr_ack); > > return intr; > } > > int msm_dp_catalog_ctrl_get_interrupt(struct msm_dp_catalog *msm_dp_catalog) > { > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > u32 intr, intr_ack; > > - intr = msm_dp_read_ahb(catalog, REG_DP_INTR_STATUS2); > + intr = msm_dp_read_ahb(msm_dp_catalog, REG_DP_INTR_STATUS2); > intr &= ~DP_INTERRUPT_STATUS2_MASK; > intr_ack = (intr & DP_INTERRUPT_STATUS2) > << DP_INTERRUPT_STATUS_ACK_SHIFT; > - msm_dp_write_ahb(catalog, REG_DP_INTR_STATUS2, > + msm_dp_write_ahb(msm_dp_catalog, REG_DP_INTR_STATUS2, > intr_ack | DP_INTERRUPT_STATUS2_MASK); > > return intr; > @@ -749,13 +676,10 @@ int msm_dp_catalog_ctrl_get_interrupt(struct msm_dp_catalog *msm_dp_catalog) > > void msm_dp_catalog_ctrl_phy_reset(struct msm_dp_catalog *msm_dp_catalog) > { > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > - > - msm_dp_write_ahb(catalog, REG_DP_PHY_CTRL, > + msm_dp_write_ahb(msm_dp_catalog, REG_DP_PHY_CTRL, > DP_PHY_CTRL_SW_RESET | DP_PHY_CTRL_SW_RESET_PLL); > usleep_range(1000, 1100); /* h/w recommended delay */ > - msm_dp_write_ahb(catalog, REG_DP_PHY_CTRL, 0x0); > + msm_dp_write_ahb(msm_dp_catalog, REG_DP_PHY_CTRL, 0x0); > } > > void msm_dp_catalog_ctrl_send_phy_pattern(struct msm_dp_catalog *msm_dp_catalog, > @@ -766,66 +690,66 @@ void msm_dp_catalog_ctrl_send_phy_pattern(struct msm_dp_catalog *msm_dp_catalog, > u32 value = 0x0; > > /* Make sure to clear the current pattern before starting a new one */ > - msm_dp_write_link(catalog, REG_DP_STATE_CTRL, 0x0); > + msm_dp_write_link(msm_dp_catalog, REG_DP_STATE_CTRL, 0x0); > > drm_dbg_dp(catalog->drm_dev, "pattern: %#x\n", pattern); > switch (pattern) { > case DP_PHY_TEST_PATTERN_D10_2: > - msm_dp_write_link(catalog, REG_DP_STATE_CTRL, > + msm_dp_write_link(msm_dp_catalog, REG_DP_STATE_CTRL, > DP_STATE_CTRL_LINK_TRAINING_PATTERN1); > break; > case DP_PHY_TEST_PATTERN_ERROR_COUNT: > value &= ~(1 << 16); > - msm_dp_write_link(catalog, REG_DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, > + msm_dp_write_link(msm_dp_catalog, REG_DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, > value); > value |= SCRAMBLER_RESET_COUNT_VALUE; > - msm_dp_write_link(catalog, REG_DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, > + msm_dp_write_link(msm_dp_catalog, REG_DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, > value); > - msm_dp_write_link(catalog, REG_DP_MAINLINK_LEVELS, > + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_LEVELS, > DP_MAINLINK_SAFE_TO_EXIT_LEVEL_2); > - msm_dp_write_link(catalog, REG_DP_STATE_CTRL, > + msm_dp_write_link(msm_dp_catalog, REG_DP_STATE_CTRL, > DP_STATE_CTRL_LINK_SYMBOL_ERR_MEASURE); > break; > case DP_PHY_TEST_PATTERN_PRBS7: > - msm_dp_write_link(catalog, REG_DP_STATE_CTRL, > + msm_dp_write_link(msm_dp_catalog, REG_DP_STATE_CTRL, > DP_STATE_CTRL_LINK_PRBS7); > break; > case DP_PHY_TEST_PATTERN_80BIT_CUSTOM: > - msm_dp_write_link(catalog, REG_DP_STATE_CTRL, > + msm_dp_write_link(msm_dp_catalog, REG_DP_STATE_CTRL, > DP_STATE_CTRL_LINK_TEST_CUSTOM_PATTERN); > /* 00111110000011111000001111100000 */ > - msm_dp_write_link(catalog, REG_DP_TEST_80BIT_CUSTOM_PATTERN_REG0, > + msm_dp_write_link(msm_dp_catalog, REG_DP_TEST_80BIT_CUSTOM_PATTERN_REG0, > 0x3E0F83E0); > /* 00001111100000111110000011111000 */ > - msm_dp_write_link(catalog, REG_DP_TEST_80BIT_CUSTOM_PATTERN_REG1, > + msm_dp_write_link(msm_dp_catalog, REG_DP_TEST_80BIT_CUSTOM_PATTERN_REG1, > 0x0F83E0F8); > /* 1111100000111110 */ > - msm_dp_write_link(catalog, REG_DP_TEST_80BIT_CUSTOM_PATTERN_REG2, > + msm_dp_write_link(msm_dp_catalog, REG_DP_TEST_80BIT_CUSTOM_PATTERN_REG2, > 0x0000F83E); > break; > case DP_PHY_TEST_PATTERN_CP2520: > - value = msm_dp_read_link(catalog, REG_DP_MAINLINK_CTRL); > + value = msm_dp_read_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL); > value &= ~DP_MAINLINK_CTRL_SW_BYPASS_SCRAMBLER; > - msm_dp_write_link(catalog, REG_DP_MAINLINK_CTRL, value); > + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL, value); > > value = DP_HBR2_ERM_PATTERN; > - msm_dp_write_link(catalog, REG_DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, > + msm_dp_write_link(msm_dp_catalog, REG_DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, > value); > value |= SCRAMBLER_RESET_COUNT_VALUE; > - msm_dp_write_link(catalog, REG_DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, > + msm_dp_write_link(msm_dp_catalog, REG_DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, > value); > - msm_dp_write_link(catalog, REG_DP_MAINLINK_LEVELS, > + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_LEVELS, > DP_MAINLINK_SAFE_TO_EXIT_LEVEL_2); > - msm_dp_write_link(catalog, REG_DP_STATE_CTRL, > + msm_dp_write_link(msm_dp_catalog, REG_DP_STATE_CTRL, > DP_STATE_CTRL_LINK_SYMBOL_ERR_MEASURE); > - value = msm_dp_read_link(catalog, REG_DP_MAINLINK_CTRL); > + value = msm_dp_read_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL); > value |= DP_MAINLINK_CTRL_ENABLE; > - msm_dp_write_link(catalog, REG_DP_MAINLINK_CTRL, value); > + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL, value); > break; > case DP_PHY_TEST_PATTERN_SEL_MASK: > - msm_dp_write_link(catalog, REG_DP_MAINLINK_CTRL, > + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL, > DP_MAINLINK_CTRL_ENABLE); > - msm_dp_write_link(catalog, REG_DP_STATE_CTRL, > + msm_dp_write_link(msm_dp_catalog, REG_DP_STATE_CTRL, > DP_STATE_CTRL_LINK_TRAINING_PATTERN4); > break; > default: > @@ -837,26 +761,21 @@ void msm_dp_catalog_ctrl_send_phy_pattern(struct msm_dp_catalog *msm_dp_catalog, > > u32 msm_dp_catalog_ctrl_read_phy_pattern(struct msm_dp_catalog *msm_dp_catalog) > { > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > - > - return msm_dp_read_link(catalog, REG_DP_MAINLINK_READY); > + return msm_dp_read_link(msm_dp_catalog, REG_DP_MAINLINK_READY); > } > > /* panel related catalog functions */ > int msm_dp_catalog_panel_timing_cfg(struct msm_dp_catalog *msm_dp_catalog, u32 total, > u32 sync_start, u32 width_blanking, u32 msm_dp_active) > { > - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, > - struct msm_dp_catalog_private, msm_dp_catalog); > u32 reg; > > - msm_dp_write_link(catalog, REG_DP_TOTAL_HOR_VER, total); > - msm_dp_write_link(catalog, REG_DP_START_HOR_VER_FROM_SYNC, sync_start); > - msm_dp_write_link(catalog, REG_DP_HSYNC_VSYNC_WIDTH_POLARITY, width_blanking); > - msm_dp_write_link(catalog, REG_DP_ACTIVE_HOR_VER, msm_dp_active); > + msm_dp_write_link(msm_dp_catalog, REG_DP_TOTAL_HOR_VER, total); > + msm_dp_write_link(msm_dp_catalog, REG_DP_START_HOR_VER_FROM_SYNC, sync_start); > + msm_dp_write_link(msm_dp_catalog, REG_DP_HSYNC_VSYNC_WIDTH_POLARITY, width_blanking); > + msm_dp_write_link(msm_dp_catalog, REG_DP_ACTIVE_HOR_VER, msm_dp_active); > > - reg = msm_dp_read_p0(catalog, MMSS_DP_INTF_CONFIG); > + reg = msm_dp_read_p0(msm_dp_catalog, MMSS_DP_INTF_CONFIG); > > if (msm_dp_catalog->wide_bus_en) > reg |= DP_INTF_CONFIG_DATABUS_WIDEN; > @@ -866,42 +785,36 @@ int msm_dp_catalog_panel_timing_cfg(struct msm_dp_catalog *msm_dp_catalog, u32 t > > DRM_DEBUG_DP("wide_bus_en=%d reg=%#x\n", msm_dp_catalog->wide_bus_en, reg); > > - msm_dp_write_p0(catalog, MMSS_DP_INTF_CONFIG, reg); > + msm_dp_write_p0(msm_dp_catalog, MMSS_DP_INTF_CONFIG, reg); > return 0; > } > > static void msm_dp_catalog_panel_send_vsc_sdp(struct msm_dp_catalog *msm_dp_catalog, struct dp_sdp *vsc_sdp) > { > - struct msm_dp_catalog_private *catalog; > u32 header[2]; > u32 val; > int i; > > - catalog = container_of(msm_dp_catalog, struct msm_dp_catalog_private, msm_dp_catalog); > - > msm_dp_utils_pack_sdp_header(&vsc_sdp->sdp_header, header); > > - msm_dp_write_link(catalog, MMSS_DP_GENERIC0_0, header[0]); > - msm_dp_write_link(catalog, MMSS_DP_GENERIC0_1, header[1]); > + msm_dp_write_link(msm_dp_catalog, MMSS_DP_GENERIC0_0, header[0]); > + msm_dp_write_link(msm_dp_catalog, MMSS_DP_GENERIC0_1, header[1]); > > for (i = 0; i < sizeof(vsc_sdp->db); i += 4) { > val = ((vsc_sdp->db[i]) | (vsc_sdp->db[i + 1] << 8) | (vsc_sdp->db[i + 2] << 16) | > (vsc_sdp->db[i + 3] << 24)); > - msm_dp_write_link(catalog, MMSS_DP_GENERIC0_2 + i, val); > + msm_dp_write_link(msm_dp_catalog, MMSS_DP_GENERIC0_2 + i, val); > } > } > > static void msm_dp_catalog_panel_update_sdp(struct msm_dp_catalog *msm_dp_catalog) > { > - struct msm_dp_catalog_private *catalog; > u32 hw_revision; > > - catalog = container_of(msm_dp_catalog, struct msm_dp_catalog_private, msm_dp_catalog); > - > hw_revision = msm_dp_catalog_hw_revision(msm_dp_catalog); > if (hw_revision < DP_HW_VERSION_1_2 && hw_revision >= DP_HW_VERSION_1_0) { > - msm_dp_write_link(catalog, MMSS_DP_SDP_CFG3, 0x01); > - msm_dp_write_link(catalog, MMSS_DP_SDP_CFG3, 0x00); > + msm_dp_write_link(msm_dp_catalog, MMSS_DP_SDP_CFG3, 0x01); > + msm_dp_write_link(msm_dp_catalog, MMSS_DP_SDP_CFG3, 0x00); > } > } > > @@ -912,15 +825,15 @@ void msm_dp_catalog_panel_enable_vsc_sdp(struct msm_dp_catalog *msm_dp_catalog, > > catalog = container_of(msm_dp_catalog, struct msm_dp_catalog_private, msm_dp_catalog); > > - cfg = msm_dp_read_link(catalog, MMSS_DP_SDP_CFG); > - cfg2 = msm_dp_read_link(catalog, MMSS_DP_SDP_CFG2); > - misc = msm_dp_read_link(catalog, REG_DP_MISC1_MISC0); > + cfg = msm_dp_read_link(msm_dp_catalog, MMSS_DP_SDP_CFG); > + cfg2 = msm_dp_read_link(msm_dp_catalog, MMSS_DP_SDP_CFG2); > + misc = msm_dp_read_link(msm_dp_catalog, REG_DP_MISC1_MISC0); > > cfg |= GEN0_SDP_EN; > - msm_dp_write_link(catalog, MMSS_DP_SDP_CFG, cfg); > + msm_dp_write_link(msm_dp_catalog, MMSS_DP_SDP_CFG, cfg); > > cfg2 |= GENERIC0_SDPSIZE_VALID; > - msm_dp_write_link(catalog, MMSS_DP_SDP_CFG2, cfg2); > + msm_dp_write_link(msm_dp_catalog, MMSS_DP_SDP_CFG2, cfg2); > > msm_dp_catalog_panel_send_vsc_sdp(msm_dp_catalog, vsc_sdp); > > @@ -930,7 +843,7 @@ void msm_dp_catalog_panel_enable_vsc_sdp(struct msm_dp_catalog *msm_dp_catalog, > drm_dbg_dp(catalog->drm_dev, "vsc sdp enable=1\n"); > > pr_debug("misc settings = 0x%x\n", misc); > - msm_dp_write_link(catalog, REG_DP_MISC1_MISC0, misc); > + msm_dp_write_link(msm_dp_catalog, REG_DP_MISC1_MISC0, misc); > > msm_dp_catalog_panel_update_sdp(msm_dp_catalog); > } > @@ -942,15 +855,15 @@ void msm_dp_catalog_panel_disable_vsc_sdp(struct msm_dp_catalog *msm_dp_catalog) > > catalog = container_of(msm_dp_catalog, struct msm_dp_catalog_private, msm_dp_catalog); > > - cfg = msm_dp_read_link(catalog, MMSS_DP_SDP_CFG); > - cfg2 = msm_dp_read_link(catalog, MMSS_DP_SDP_CFG2); > - misc = msm_dp_read_link(catalog, REG_DP_MISC1_MISC0); > + cfg = msm_dp_read_link(msm_dp_catalog, MMSS_DP_SDP_CFG); > + cfg2 = msm_dp_read_link(msm_dp_catalog, MMSS_DP_SDP_CFG2); > + misc = msm_dp_read_link(msm_dp_catalog, REG_DP_MISC1_MISC0); > > cfg &= ~GEN0_SDP_EN; > - msm_dp_write_link(catalog, MMSS_DP_SDP_CFG, cfg); > + msm_dp_write_link(msm_dp_catalog, MMSS_DP_SDP_CFG, cfg); > > cfg2 &= ~GENERIC0_SDPSIZE_VALID; > - msm_dp_write_link(catalog, MMSS_DP_SDP_CFG2, cfg2); > + msm_dp_write_link(msm_dp_catalog, MMSS_DP_SDP_CFG2, cfg2); > > /* switch back to MSA */ > misc &= ~DP_MISC1_VSC_SDP; > @@ -958,7 +871,7 @@ void msm_dp_catalog_panel_disable_vsc_sdp(struct msm_dp_catalog *msm_dp_catalog) > drm_dbg_dp(catalog->drm_dev, "vsc sdp enable=0\n"); > > pr_debug("misc settings = 0x%x\n", misc); > - msm_dp_write_link(catalog, REG_DP_MISC1_MISC0, misc); > + msm_dp_write_link(msm_dp_catalog, REG_DP_MISC1_MISC0, misc); > > msm_dp_catalog_panel_update_sdp(msm_dp_catalog); > } > @@ -1055,15 +968,15 @@ static void __iomem *msm_dp_ioremap(struct platform_device *pdev, int idx, size_ > > static int msm_dp_catalog_get_io(struct msm_dp_catalog_private *catalog) > { > + struct msm_dp_catalog *msm_dp_catalog = &catalog->msm_dp_catalog; > struct platform_device *pdev = to_platform_device(catalog->dev); > - struct dss_io_data *dss = &catalog->io; > > - dss->ahb.base = msm_dp_ioremap(pdev, 0, &dss->ahb.len); > - if (IS_ERR(dss->ahb.base)) > - return PTR_ERR(dss->ahb.base); > + msm_dp_catalog->ahb_base = msm_dp_ioremap(pdev, 0, &msm_dp_catalog->ahb_len); > + if (IS_ERR(msm_dp_catalog->ahb_base)) > + return PTR_ERR(msm_dp_catalog->ahb_base); > > - dss->aux.base = msm_dp_ioremap(pdev, 1, &dss->aux.len); > - if (IS_ERR(dss->aux.base)) { > + msm_dp_catalog->aux_base = msm_dp_ioremap(pdev, 1, &msm_dp_catalog->aux_len); > + if (IS_ERR(msm_dp_catalog->aux_base)) { > /* > * The initial binding had a single reg, but in order to > * support variation in the sub-region sizes this was split. > @@ -1071,34 +984,34 @@ static int msm_dp_catalog_get_io(struct msm_dp_catalog_private *catalog) > * reg is specified, so fill in the sub-region offsets and > * lengths based on this single region. > */ > - if (PTR_ERR(dss->aux.base) == -EINVAL) { > - if (dss->ahb.len < DP_DEFAULT_P0_OFFSET + DP_DEFAULT_P0_SIZE) { > + if (PTR_ERR(msm_dp_catalog->aux_base) == -EINVAL) { > + if (msm_dp_catalog->ahb_len < DP_DEFAULT_P0_OFFSET + DP_DEFAULT_P0_SIZE) { > DRM_ERROR("legacy memory region not large enough\n"); > return -EINVAL; > } > > - dss->ahb.len = DP_DEFAULT_AHB_SIZE; > - dss->aux.base = dss->ahb.base + DP_DEFAULT_AUX_OFFSET; > - dss->aux.len = DP_DEFAULT_AUX_SIZE; > - dss->link.base = dss->ahb.base + DP_DEFAULT_LINK_OFFSET; > - dss->link.len = DP_DEFAULT_LINK_SIZE; > - dss->p0.base = dss->ahb.base + DP_DEFAULT_P0_OFFSET; > - dss->p0.len = DP_DEFAULT_P0_SIZE; > + msm_dp_catalog->ahb_len = DP_DEFAULT_AHB_SIZE; > + msm_dp_catalog->aux_base = msm_dp_catalog->ahb_base + DP_DEFAULT_AUX_OFFSET; > + msm_dp_catalog->aux_len = DP_DEFAULT_AUX_SIZE; > + msm_dp_catalog->link_base = msm_dp_catalog->ahb_base + DP_DEFAULT_LINK_OFFSET; > + msm_dp_catalog->link_len = DP_DEFAULT_LINK_SIZE; > + msm_dp_catalog->p0_base = msm_dp_catalog->ahb_base + DP_DEFAULT_P0_OFFSET; > + msm_dp_catalog->p0_len = DP_DEFAULT_P0_SIZE; > } else { > - DRM_ERROR("unable to remap aux region: %pe\n", dss->aux.base); > - return PTR_ERR(dss->aux.base); > + DRM_ERROR("unable to remap aux region: %pe\n", msm_dp_catalog->aux_base); > + return PTR_ERR(msm_dp_catalog->aux_base); > } > } else { > - dss->link.base = msm_dp_ioremap(pdev, 2, &dss->link.len); > - if (IS_ERR(dss->link.base)) { > - DRM_ERROR("unable to remap link region: %pe\n", dss->link.base); > - return PTR_ERR(dss->link.base); > + msm_dp_catalog->link_base = msm_dp_ioremap(pdev, 2, &msm_dp_catalog->link_len); > + if (IS_ERR(msm_dp_catalog->link_base)) { > + DRM_ERROR("unable to remap link region: %pe\n", msm_dp_catalog->link_base); > + return PTR_ERR(msm_dp_catalog->link_base); > } > > - dss->p0.base = msm_dp_ioremap(pdev, 3, &dss->p0.len); > - if (IS_ERR(dss->p0.base)) { > - DRM_ERROR("unable to remap p0 region: %pe\n", dss->p0.base); > - return PTR_ERR(dss->p0.base); > + msm_dp_catalog->p0_base = msm_dp_ioremap(pdev, 3, &msm_dp_catalog->p0_len); > + if (IS_ERR(msm_dp_catalog->p0_base)) { > + DRM_ERROR("unable to remap p0 region: %pe\n", msm_dp_catalog->p0_base); > + return PTR_ERR(msm_dp_catalog->p0_base); > } > } > > @@ -1135,7 +1048,7 @@ u32 msm_dp_catalog_audio_get_header(struct msm_dp_catalog *msm_dp_catalog, > > sdp_map = catalog->audio_map; > > - return msm_dp_read_link(catalog, sdp_map[sdp][header]); > + return msm_dp_read_link(msm_dp_catalog, sdp_map[sdp][header]); > } > > void msm_dp_catalog_audio_set_header(struct msm_dp_catalog *msm_dp_catalog, > @@ -1154,7 +1067,7 @@ void msm_dp_catalog_audio_set_header(struct msm_dp_catalog *msm_dp_catalog, > > sdp_map = catalog->audio_map; > > - msm_dp_write_link(catalog, sdp_map[sdp][header], data); > + msm_dp_write_link(msm_dp_catalog, sdp_map[sdp][header], data); > } > > void msm_dp_catalog_audio_config_acr(struct msm_dp_catalog *msm_dp_catalog, u32 select) > @@ -1173,7 +1086,7 @@ void msm_dp_catalog_audio_config_acr(struct msm_dp_catalog *msm_dp_catalog, u32 > drm_dbg_dp(catalog->drm_dev, "select: %#x, acr_ctrl: %#x\n", > select, acr_ctrl); > > - msm_dp_write_link(catalog, MMSS_DP_AUDIO_ACR_CTRL, acr_ctrl); > + msm_dp_write_link(msm_dp_catalog, MMSS_DP_AUDIO_ACR_CTRL, acr_ctrl); > } > > void msm_dp_catalog_audio_enable(struct msm_dp_catalog *msm_dp_catalog, bool enable) > @@ -1187,7 +1100,7 @@ void msm_dp_catalog_audio_enable(struct msm_dp_catalog *msm_dp_catalog, bool ena > catalog = container_of(msm_dp_catalog, > struct msm_dp_catalog_private, msm_dp_catalog); > > - audio_ctrl = msm_dp_read_link(catalog, MMSS_DP_AUDIO_CFG); > + audio_ctrl = msm_dp_read_link(msm_dp_catalog, MMSS_DP_AUDIO_CFG); > > if (enable) > audio_ctrl |= BIT(0); > @@ -1196,7 +1109,7 @@ void msm_dp_catalog_audio_enable(struct msm_dp_catalog *msm_dp_catalog, bool ena > > drm_dbg_dp(catalog->drm_dev, "dp_audio_cfg = 0x%x\n", audio_ctrl); > > - msm_dp_write_link(catalog, MMSS_DP_AUDIO_CFG, audio_ctrl); > + msm_dp_write_link(msm_dp_catalog, MMSS_DP_AUDIO_CFG, audio_ctrl); > /* make sure audio engine is disabled */ > wmb(); > } > @@ -1213,7 +1126,7 @@ void msm_dp_catalog_audio_config_sdp(struct msm_dp_catalog *msm_dp_catalog) > catalog = container_of(msm_dp_catalog, > struct msm_dp_catalog_private, msm_dp_catalog); > > - sdp_cfg = msm_dp_read_link(catalog, MMSS_DP_SDP_CFG); > + sdp_cfg = msm_dp_read_link(msm_dp_catalog, MMSS_DP_SDP_CFG); > /* AUDIO_TIMESTAMP_SDP_EN */ > sdp_cfg |= BIT(1); > /* AUDIO_STREAM_SDP_EN */ > @@ -1227,9 +1140,9 @@ void msm_dp_catalog_audio_config_sdp(struct msm_dp_catalog *msm_dp_catalog) > > drm_dbg_dp(catalog->drm_dev, "sdp_cfg = 0x%x\n", sdp_cfg); > > - msm_dp_write_link(catalog, MMSS_DP_SDP_CFG, sdp_cfg); > + msm_dp_write_link(msm_dp_catalog, MMSS_DP_SDP_CFG, sdp_cfg); > > - sdp_cfg2 = msm_dp_read_link(catalog, MMSS_DP_SDP_CFG2); > + sdp_cfg2 = msm_dp_read_link(msm_dp_catalog, MMSS_DP_SDP_CFG2); > /* IFRM_REGSRC -> Do not use reg values */ > sdp_cfg2 &= ~BIT(0); > /* AUDIO_STREAM_HB3_REGSRC-> Do not use reg values */ > @@ -1237,7 +1150,7 @@ void msm_dp_catalog_audio_config_sdp(struct msm_dp_catalog *msm_dp_catalog) > > drm_dbg_dp(catalog->drm_dev, "sdp_cfg2 = 0x%x\n", sdp_cfg2); > > - msm_dp_write_link(catalog, MMSS_DP_SDP_CFG2, sdp_cfg2); > + msm_dp_write_link(msm_dp_catalog, MMSS_DP_SDP_CFG2, sdp_cfg2); > } > > void msm_dp_catalog_audio_init(struct msm_dp_catalog *msm_dp_catalog) > @@ -1292,7 +1205,7 @@ void msm_dp_catalog_audio_sfe_level(struct msm_dp_catalog *msm_dp_catalog, u32 s > catalog = container_of(msm_dp_catalog, > struct msm_dp_catalog_private, msm_dp_catalog); > > - mainlink_levels = msm_dp_read_link(catalog, REG_DP_MAINLINK_LEVELS); > + mainlink_levels = msm_dp_read_link(msm_dp_catalog, REG_DP_MAINLINK_LEVELS); > mainlink_levels &= 0xFE0; > mainlink_levels |= safe_to_exit_level; > > @@ -1300,5 +1213,5 @@ void msm_dp_catalog_audio_sfe_level(struct msm_dp_catalog *msm_dp_catalog, u32 s > "mainlink_level = 0x%x, safe_to_exit_level = 0x%x\n", > mainlink_levels, safe_to_exit_level); > > - msm_dp_write_link(catalog, REG_DP_MAINLINK_LEVELS, mainlink_levels); > + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_LEVELS, mainlink_levels); > } > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h > index 62a401d8f75a6af06445a42af657d65e3fe542c5..13486c9c8703748e69e846be681951368df0a29e 100644 > --- a/drivers/gpu/drm/msm/dp/dp_catalog.h > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h > @@ -49,6 +49,18 @@ enum msm_dp_catalog_audio_header_type { > > struct msm_dp_catalog { > bool wide_bus_en; > + > + void __iomem *ahb_base; > + size_t ahb_len; > + > + void __iomem *aux_base; > + size_t aux_len; > + > + void __iomem *link_base; > + size_t link_len; > + > + void __iomem *p0_base; > + size_t p0_len; > }; > > /* Debug module */ >
On Thu, 12 Dec 2024 at 04:59, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 12/11/2024 3:41 PM, Dmitry Baryshkov wrote: > > Having I/O regions inside a msm_dp_catalog_private() results in extra > > layers of one-line wrappers for accessing the data. Move I/O region base > > and size to the globally visible struct msm_dp_catalog. > > > > Reviewed-by: Stephen Boyd <swboyd@chromium.org> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/msm/dp/dp_catalog.c | 457 +++++++++++++++--------------------- > > drivers/gpu/drm/msm/dp/dp_catalog.h | 12 + > > 2 files changed, 197 insertions(+), 272 deletions(-) > > > > > Fundamentally, the whole point of catalog was that it needs to be the > only place where we want to access the registers. Thats how this really > started. > > This pre-dates my time with the DP driver but as I understand thats what > it was for. > > Basically separating out the logical abstraction vs actual register writes . > > If there are hardware sequence differences within the controller reset > OR any other register offsets which moved around, catalog should have > been able to absorb it without that spilling over to all the layers. > > So for example, if we call dp_ctrl_reset() --> ctrl->catalog->reset_ctrl() > > Then the reset_ctrl op of the catalog should manage any controller > version differences within the reset sequence. The problem is that the register-level writes are usually not the best abstraction. So, instead of designing the code around dp_catalog I'd prefer to see actual hw / programming changes first. > > We do not use or have catalog ops today so it looks redundant as we just > call the dp_catalog APIs directly but that was really the intention. > > Another reason which was behind this split but not applicable to current > upstream driver is that the AUX is part of the PHY driver in upstream > but in downstream, that remains a part of catalog and as we know the AUX > component keeps changing with chipsets especially the settings. That was > the reason of keeping catalog separate and the only place which should > deal with registers and not the entire DP driver. > > The second point seems not applicable to this driver but first point > still is. I do admit there is re-direction like ctrl->catalog > instead of just writing it within dp_ctrl itself but the redirection was > only because ctrl layers were not really meant to deal with the register > programming. So for example, now with patch 7 of this series every > register being written to i exposed in dp_ctrl.c and likewise for other > files. That seems unnecessary. Because if we do end up with some > variants which need separate registers written, then we will now have to > end up touching every file as opposed to only touching dp_catalog. Yes. I think that it's a bonus, not a problem. We end up touching the files that are actually changed, so we see what is happening. Quite frequently register changes are paired with the functionality changes. For example (a very simple and dumb one), when designing code around dp_catalog you ended up adding separate _p1 handlers. Doing that from the data source point of view demands adding a stream_id param. In the DPU driver we also have version-related conditionals in the HW modules rather than pushing all data access to dpu_hw_catalog.c & counterparts. I think it's better to make DP driver reflect DPU rather than keeping a separate wrapper for no particular reason (note, DPU has hardware abstractions, but on a block level, not on a register level).
On 12/12/2024 12:52 AM, Dmitry Baryshkov wrote: > On Thu, 12 Dec 2024 at 04:59, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 12/11/2024 3:41 PM, Dmitry Baryshkov wrote: >>> Having I/O regions inside a msm_dp_catalog_private() results in extra >>> layers of one-line wrappers for accessing the data. Move I/O region base >>> and size to the globally visible struct msm_dp_catalog. >>> >>> Reviewed-by: Stephen Boyd <swboyd@chromium.org> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/dp/dp_catalog.c | 457 +++++++++++++++--------------------- >>> drivers/gpu/drm/msm/dp/dp_catalog.h | 12 + >>> 2 files changed, 197 insertions(+), 272 deletions(-) >>> >> >> >> Fundamentally, the whole point of catalog was that it needs to be the >> only place where we want to access the registers. Thats how this really >> started. >> >> This pre-dates my time with the DP driver but as I understand thats what >> it was for. >> >> Basically separating out the logical abstraction vs actual register writes . >> >> If there are hardware sequence differences within the controller reset >> OR any other register offsets which moved around, catalog should have >> been able to absorb it without that spilling over to all the layers. >> >> So for example, if we call dp_ctrl_reset() --> ctrl->catalog->reset_ctrl() >> >> Then the reset_ctrl op of the catalog should manage any controller >> version differences within the reset sequence. > > The problem is that the register-level writes are usually not the best > abstraction. So, instead of designing the code around dp_catalog I'd > prefer to see actual hw / programming changes first. > So thats the issue here. If we did end up with registers and sequences different for controller versions, the ctrl layer was expected to just call a reset() op for example similar to the DPU example you gave. And as you rightly noted, the dpu_hw_xxx files only expose the ops based on version and the upper layers were supposed to just call into the ops without knowing the register level details. Thats pretty much what dp_ctrl tried to do here. We did not want to expose all the register defines in those layers. This series is doing exactly opposite of that. >> >> We do not use or have catalog ops today so it looks redundant as we just >> call the dp_catalog APIs directly but that was really the intention. >> >> Another reason which was behind this split but not applicable to current >> upstream driver is that the AUX is part of the PHY driver in upstream >> but in downstream, that remains a part of catalog and as we know the AUX >> component keeps changing with chipsets especially the settings. That was >> the reason of keeping catalog separate and the only place which should >> deal with registers and not the entire DP driver. >> >> The second point seems not applicable to this driver but first point >> still is. I do admit there is re-direction like ctrl->catalog >> instead of just writing it within dp_ctrl itself but the redirection was >> only because ctrl layers were not really meant to deal with the register >> programming. So for example, now with patch 7 of this series every >> register being written to i exposed in dp_ctrl.c and likewise for other >> files. That seems unnecessary. Because if we do end up with some >> variants which need separate registers written, then we will now have to >> end up touching every file as opposed to only touching dp_catalog. > > Yes. I think that it's a bonus, not a problem. We end up touching the > files that are actually changed, so we see what is happening. Quite > frequently register changes are paired with the functionality changes. > Not exactly. Why should dp_ctrl really know that some register offset or some block shift happened for example. It only needs to know when to reset the hardware and not how. Thats the separation getting broken with this. > For example (a very simple and dumb one), when designing code around > dp_catalog you ended up adding separate _p1 handlers. > Doing that from the data source point of view demands adding a stream_id param. > I have not checked your comment on that series here but if your concern is stream_id should not be stored in the catalog but just passed, thats fine, we can change it. stream_id as a param is needed anyway because the register programming layer needs to know which offset to use. This series is not mitigating that fact. > In the DPU driver we also have version-related conditionals in the HW > modules rather than pushing all data access to dpu_hw_catalog.c & > counterparts. The dpu_hw_catalog.c and the dp_catalog.c are not the right files to compare with each other. dp_catalog.c should be compared with dpu_hw_xxx.c and as you noted, DPU version dependencies are handled in those files only and not all over the files like what this series is doing. > I think it's better to make DP driver reflect DPU rather than keeping > a separate wrapper for no particular reason (note, DPU has hardware > abstractions, but on a block level, not on a register level). > Thats the issue here. DPU hardware blocks are arranged according to the sub-blocks both in the software interface document and hence the code matches it file-by-file. DP registers are grouped by clock domains and the file separation we have today does not match that anyway. Hence grouping link registers writes or pixel clock register writes into dp_ctrl is also not correct that way. Let catalog handle that separation internally which it already does.
On Thu, 12 Dec 2024 at 21:15, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 12/12/2024 12:52 AM, Dmitry Baryshkov wrote: > > On Thu, 12 Dec 2024 at 04:59, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> > >> > >> On 12/11/2024 3:41 PM, Dmitry Baryshkov wrote: > >>> Having I/O regions inside a msm_dp_catalog_private() results in extra > >>> layers of one-line wrappers for accessing the data. Move I/O region base > >>> and size to the globally visible struct msm_dp_catalog. > >>> > >>> Reviewed-by: Stephen Boyd <swboyd@chromium.org> > >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >>> --- > >>> drivers/gpu/drm/msm/dp/dp_catalog.c | 457 +++++++++++++++--------------------- > >>> drivers/gpu/drm/msm/dp/dp_catalog.h | 12 + > >>> 2 files changed, 197 insertions(+), 272 deletions(-) > >>> > >> > >> > >> Fundamentally, the whole point of catalog was that it needs to be the > >> only place where we want to access the registers. Thats how this really > >> started. > >> > >> This pre-dates my time with the DP driver but as I understand thats what > >> it was for. > >> > >> Basically separating out the logical abstraction vs actual register writes . > >> > >> If there are hardware sequence differences within the controller reset > >> OR any other register offsets which moved around, catalog should have > >> been able to absorb it without that spilling over to all the layers. > >> > >> So for example, if we call dp_ctrl_reset() --> ctrl->catalog->reset_ctrl() > >> > >> Then the reset_ctrl op of the catalog should manage any controller > >> version differences within the reset sequence. > > > > The problem is that the register-level writes are usually not the best > > abstraction. So, instead of designing the code around dp_catalog I'd > > prefer to see actual hw / programming changes first. > > > > So thats the issue here. If we did end up with registers and sequences > different for controller versions, the ctrl layer was expected to just > call a reset() op for example similar to the DPU example you gave. And > as you rightly noted, the dpu_hw_xxx files only expose the ops based on > version and the upper layers were supposed to just call into the ops > without knowing the register level details. Thats pretty much what > dp_ctrl tried to do here. We did not want to expose all the register > defines in those layers. This series is doing exactly opposite of that. We don't have the issue up to now, even though we support DP controllers since SDM845 up to SM8650 and X1E80100. The SDE driver has v200 vs v420 catalog files for PHY programming, the rest of the functions are common. So, for me it looks like a preparation for the imaginary case that didn't come to existence up to now. So, yes. I want to get rid of extra useless indirection and I want to expose register sequences in those layers. > > >> > >> We do not use or have catalog ops today so it looks redundant as we just > >> call the dp_catalog APIs directly but that was really the intention. > >> > >> Another reason which was behind this split but not applicable to current > >> upstream driver is that the AUX is part of the PHY driver in upstream > >> but in downstream, that remains a part of catalog and as we know the AUX > >> component keeps changing with chipsets especially the settings. That was > >> the reason of keeping catalog separate and the only place which should > >> deal with registers and not the entire DP driver. > >> > >> The second point seems not applicable to this driver but first point > >> still is. I do admit there is re-direction like ctrl->catalog > >> instead of just writing it within dp_ctrl itself but the redirection was > >> only because ctrl layers were not really meant to deal with the register > >> programming. So for example, now with patch 7 of this series every > >> register being written to i exposed in dp_ctrl.c and likewise for other > >> files. That seems unnecessary. Because if we do end up with some > >> variants which need separate registers written, then we will now have to > >> end up touching every file as opposed to only touching dp_catalog. > > > > Yes. I think that it's a bonus, not a problem. We end up touching the > > files that are actually changed, so we see what is happening. Quite > > frequently register changes are paired with the functionality changes. > > > > Not exactly. Why should dp_ctrl really know that some register offset or > some block shift happened for example. It only needs to know when to > reset the hardware and not how. Thats the separation getting broken with > this. Yes. And I'm removing that separation very intentionally. If one is looking for AUX programming, they should be looking into dp_aux only, not dp_aux & dp_catalog. Likewise all audio code should be in dp_audio. By using dp_catalog we ended up with a very very very bad abstraction of msm_dp_catalog_audio_get_header() / msm_dp_catalog_audio_set_header() / enum msm_dp_catalog_audio_sdp_type. Just because reads & writes should go through the catalog. For dp_panel likewise there is no need to look into some other source file to follow the register sequences. It can all be contained within dp_panel.c, helping one to understand the code. Last, but not least. Code complexity. dp_catalog.c consists of 1340 lines, covering different submodules. It is hard to follow it in this way. > > > For example (a very simple and dumb one), when designing code around > > dp_catalog you ended up adding separate _p1 handlers. > > Doing that from the data source point of view demands adding a stream_id param. > > > > I have not checked your comment on that series here but if your concern This is really a bad cadence. I have provided most of the feedback almost a week ago. > is stream_id should not be stored in the catalog but just passed, thats > fine, we can change it. stream_id as a param is needed anyway because > the register programming layer needs to know which offset to use. This > series is not mitigating that fact. No, my concern was that you have been adding separate _p1() functions which are a duplicate of _p0() counterparts. When one looks at the dp_catalog.c it is logical: there are two different register areas, so there are two distinct sets of functions. If one starts looking from the dp_panel point of view, it's obvious that there should be a single msm_dp_write_stream() function which accepts stream_id and then multiplexes it to go to p0 or p1. > > > In the DPU driver we also have version-related conditionals in the HW > > modules rather than pushing all data access to dpu_hw_catalog.c & > > counterparts. > > The dpu_hw_catalog.c and the dp_catalog.c are not the right files to > compare with each other. dp_catalog.c should be compared with > dpu_hw_xxx.c and as you noted, DPU version dependencies are handled in > those files only and not all over the files like what this series is doing. Not really. dpu_encoder_phys_cmd_init() checks for the core_major_ver. Let me see if other files check for the version under the hood. Also as you wrote, there are multiple dpu_hw_xxx.c files, each handling register issues on its own. We don't have a single file which keeps all such differences in one place. Last, but not least, in the DPU driver there are actual differences between generations, which require different code paths. In the DP driver there are none. > > > I think it's better to make DP driver reflect DPU rather than keeping > > a separate wrapper for no particular reason (note, DPU has hardware > > abstractions, but on a block level, not on a register level). > > > > Thats the issue here. DPU hardware blocks are arranged according to the > sub-blocks both in the software interface document and hence the code > matches it file-by-file. DP registers are grouped by clock domains and > the file separation we have today does not match that anyway. Hence > grouping link registers writes or pixel clock register writes into > dp_ctrl is also not correct that way. Let catalog handle that separation > internally which it already does. I'd say, dp_panel, dp_audio and dp_link are already pretty self-contained. I was hoping to look at dp_display vs dp_drm later on, once the HPD issue gets resolved. Only dp_ctrl is not that logical from my point of view.
Hi Dmitry On 12/12/2024 3:09 PM, Dmitry Baryshkov wrote: > On Thu, 12 Dec 2024 at 21:15, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 12/12/2024 12:52 AM, Dmitry Baryshkov wrote: >>> On Thu, 12 Dec 2024 at 04:59, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 12/11/2024 3:41 PM, Dmitry Baryshkov wrote: >>>>> Having I/O regions inside a msm_dp_catalog_private() results in extra >>>>> layers of one-line wrappers for accessing the data. Move I/O region base >>>>> and size to the globally visible struct msm_dp_catalog. >>>>> >>>>> Reviewed-by: Stephen Boyd <swboyd@chromium.org> >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>>> --- >>>>> drivers/gpu/drm/msm/dp/dp_catalog.c | 457 +++++++++++++++--------------------- >>>>> drivers/gpu/drm/msm/dp/dp_catalog.h | 12 + >>>>> 2 files changed, 197 insertions(+), 272 deletions(-) >>>>> >>>> >>>> >>>> Fundamentally, the whole point of catalog was that it needs to be the >>>> only place where we want to access the registers. Thats how this really >>>> started. >>>> >>>> This pre-dates my time with the DP driver but as I understand thats what >>>> it was for. >>>> >>>> Basically separating out the logical abstraction vs actual register writes . >>>> >>>> If there are hardware sequence differences within the controller reset >>>> OR any other register offsets which moved around, catalog should have >>>> been able to absorb it without that spilling over to all the layers. >>>> >>>> So for example, if we call dp_ctrl_reset() --> ctrl->catalog->reset_ctrl() >>>> >>>> Then the reset_ctrl op of the catalog should manage any controller >>>> version differences within the reset sequence. >>> >>> The problem is that the register-level writes are usually not the best >>> abstraction. So, instead of designing the code around dp_catalog I'd >>> prefer to see actual hw / programming changes first. >>> >> >> So thats the issue here. If we did end up with registers and sequences >> different for controller versions, the ctrl layer was expected to just >> call a reset() op for example similar to the DPU example you gave. And >> as you rightly noted, the dpu_hw_xxx files only expose the ops based on >> version and the upper layers were supposed to just call into the ops >> without knowing the register level details. Thats pretty much what >> dp_ctrl tried to do here. We did not want to expose all the register >> defines in those layers. This series is doing exactly opposite of that. > > We don't have the issue up to now, even though we support DP > controllers since SDM845 up to SM8650 and X1E80100. The SDE driver has > v200 vs v420 catalog files for PHY programming, the rest of the > functions are common. So, for me it looks like a preparation for the > imaginary case that didn't come to existence up to now. > So, yes. I want to get rid of extra useless indirection and I want to > expose register sequences in those layers. > Yes because PHY programming is managed in the PHY driver today and does not go through catalog whereas in SDE driver it does, I do not have any other concrete example to give you which exists in the current code where sequence changes across chipset variants for DP controller and since I certainly cannot discuss how things can evolve moving forward, as usual, I have to accept it as one of those things which is not used today. So yes, I guess the register sequencing point changing across chipset variants, does not have a good example which I can really share. But exposing register sequences within the same file, I am not too sure about that. For example, you can take a look at dp_catalog_panel_config_hdr in the SDE code OR even msm_dp_catalog_panel_enable_vsc_sdp in the current upstream code. Why should this entire sequence be exposed to the dp_panel layer? For smaller functions which are one-liners the redirection seems redundant but when the sequence is bigger like in the examples I gave, the logical Vs register sequence separation grows. Thats where the dp_catalog came from. >> >>>> >>>> We do not use or have catalog ops today so it looks redundant as we just >>>> call the dp_catalog APIs directly but that was really the intention. >>>> >>>> Another reason which was behind this split but not applicable to current >>>> upstream driver is that the AUX is part of the PHY driver in upstream >>>> but in downstream, that remains a part of catalog and as we know the AUX >>>> component keeps changing with chipsets especially the settings. That was >>>> the reason of keeping catalog separate and the only place which should >>>> deal with registers and not the entire DP driver. >>>> >>>> The second point seems not applicable to this driver but first point >>>> still is. I do admit there is re-direction like ctrl->catalog >>>> instead of just writing it within dp_ctrl itself but the redirection was >>>> only because ctrl layers were not really meant to deal with the register >>>> programming. So for example, now with patch 7 of this series every >>>> register being written to i exposed in dp_ctrl.c and likewise for other >>>> files. That seems unnecessary. Because if we do end up with some >>>> variants which need separate registers written, then we will now have to >>>> end up touching every file as opposed to only touching dp_catalog. >>> >>> Yes. I think that it's a bonus, not a problem. We end up touching the >>> files that are actually changed, so we see what is happening. Quite >>> frequently register changes are paired with the functionality changes. >>> >> >> Not exactly. Why should dp_ctrl really know that some register offset or >> some block shift happened for example. It only needs to know when to >> reset the hardware and not how. Thats the separation getting broken with >> this. > > Yes. And I'm removing that separation very intentionally. If one is > looking for AUX programming, they should be looking into dp_aux only, > not dp_aux & dp_catalog. Likewise all audio code should be in > dp_audio. By using dp_catalog we ended up with a very very very bad > abstraction of msm_dp_catalog_audio_get_header() / > msm_dp_catalog_audio_set_header() / enum > msm_dp_catalog_audio_sdp_type. Just because reads & writes should go > through the catalog. No, I think this is where there is some correction needed. the get_header() / set_header() was done not because all writes need to go through catalog but because the audio headers were thought to be written only one header at a time and we had thought that read-modify-write had to be done to preserve the bytes. And when we have to do only one header at a time and because two headers map to one register, catalog had to end up managing an audio_map. Now, after checking where it came from as I commented on that patch, this requirement was not a functional one but was just trying to preserve the pre-silicon validation scripts sequence, this part of it can be dropped. So no need of get_header() / set_header() and an audio_map. Now all registers going through catalog is another thing which we are still discussing here. > For dp_panel likewise there is no need to look into some other source > file to follow the register sequences. It can all be contained within > dp_panel.c, helping one to understand the code. > > Last, but not least. Code complexity. dp_catalog.c consists of 1340 > lines, covering different submodules. It is hard to follow it in this > way. > Its just a question of spreading up the functions all over, not reducing code complexity. So yes, it reduces the file size of dp_catalog whereas increases that of others. Code complexity impact due to that is subjective. >> >>> For example (a very simple and dumb one), when designing code around >>> dp_catalog you ended up adding separate _p1 handlers. >>> Doing that from the data source point of view demands adding a stream_id param. >>> >> >> I have not checked your comment on that series here but if your concern > > This is really a bad cadence. I have provided most of the feedback > almost a week ago. > Yes, was a very tight week trying to enable upstream developers to land their platforms such as QCS615 by fixing platform specific dpu things and had the fixes cycle this week too so as a result my own feature took a bit of a hit this week :( >> is stream_id should not be stored in the catalog but just passed, thats >> fine, we can change it. stream_id as a param is needed anyway because >> the register programming layer needs to know which offset to use. This >> series is not mitigating that fact. > > No, my concern was that you have been adding separate _p1() functions > which are a duplicate of _p0() counterparts. When one looks at the > dp_catalog.c it is logical: there are two different register areas, so > there are two distinct sets of functions. If one starts looking from > the dp_panel point of view, it's obvious that there should be a single > msm_dp_write_stream() function which accepts stream_id and then > multiplexes it to go to p0 or p1. > Your multiplexing suggestion of adding a msm_dp_read_pn/msm_dp_write_pn by passing a stream_id can be done even with current dp_catalog intact as it will help reduce storing the stream_id in the dp_catalog. So its a valid suggestion and can be implemented even in the current code and not tied to the fact that register writing is done in dp_catalog or dp_panel. >> >>> In the DPU driver we also have version-related conditionals in the HW >>> modules rather than pushing all data access to dpu_hw_catalog.c & >>> counterparts. >> >> The dpu_hw_catalog.c and the dp_catalog.c are not the right files to >> compare with each other. dp_catalog.c should be compared with >> dpu_hw_xxx.c and as you noted, DPU version dependencies are handled in >> those files only and not all over the files like what this series is doing. > > Not really. dpu_encoder_phys_cmd_init() checks for the core_major_ver. > Let me see if other files check for the version under the hood. > Well, thats because only cmd mode panel cares about TE. No other files from what I checked. > Also as you wrote, there are multiple dpu_hw_xxx.c files, each > handling register issues on its own. We don't have a single file which > keeps all such differences in one place. > Thats because of the way the registers are laid our in the SW interface document aligns nicely with the file split we have in the DPU even when the first DPU post happened. But I still dont think its a fair comparison. If you really had to go deeper into this thought, then even dp_reg.h should be broken down into smaller headers because the offsets in dpu_hw_*** files are relevant only to those files but after this change all DP files must include dp_reg.h even though they will not be using all of the offsets. Since current code was already doing that, which it didnt have to as dp_Catalog was the only one writing all registers, this went unnoticed. > Last, but not least, in the DPU driver there are actual differences > between generations, which require different code paths. In the DP > driver there are none. > >> >>> I think it's better to make DP driver reflect DPU rather than keeping >>> a separate wrapper for no particular reason (note, DPU has hardware >>> abstractions, but on a block level, not on a register level). >>> >> >> Thats the issue here. DPU hardware blocks are arranged according to the >> sub-blocks both in the software interface document and hence the code >> matches it file-by-file. DP registers are grouped by clock domains and >> the file separation we have today does not match that anyway. Hence >> grouping link registers writes or pixel clock register writes into >> dp_ctrl is also not correct that way. Let catalog handle that separation >> internally which it already does. > > I'd say, dp_panel, dp_audio and dp_link are already pretty > self-contained. I was hoping to look at dp_display vs dp_drm later on, > once the HPD issue gets resolved. Only dp_ctrl is not that logical > from my point of view. >
On Sat, 14 Dec 2024 at 22:53, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > Hi Dmitry > > On 12/12/2024 3:09 PM, Dmitry Baryshkov wrote: > > On Thu, 12 Dec 2024 at 21:15, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> > >> > >> On 12/12/2024 12:52 AM, Dmitry Baryshkov wrote: > >>> On Thu, 12 Dec 2024 at 04:59, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >>>> > >>>> > >>>> > >>>> On 12/11/2024 3:41 PM, Dmitry Baryshkov wrote: > >>>>> Having I/O regions inside a msm_dp_catalog_private() results in extra > >>>>> layers of one-line wrappers for accessing the data. Move I/O region base > >>>>> and size to the globally visible struct msm_dp_catalog. > >>>>> > >>>>> Reviewed-by: Stephen Boyd <swboyd@chromium.org> > >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >>>>> --- > >>>>> drivers/gpu/drm/msm/dp/dp_catalog.c | 457 +++++++++++++++--------------------- > >>>>> drivers/gpu/drm/msm/dp/dp_catalog.h | 12 + > >>>>> 2 files changed, 197 insertions(+), 272 deletions(-) > >>>>> > >>>> > >>>> > >>>> Fundamentally, the whole point of catalog was that it needs to be the > >>>> only place where we want to access the registers. Thats how this really > >>>> started. > >>>> > >>>> This pre-dates my time with the DP driver but as I understand thats what > >>>> it was for. > >>>> > >>>> Basically separating out the logical abstraction vs actual register writes . > >>>> > >>>> If there are hardware sequence differences within the controller reset > >>>> OR any other register offsets which moved around, catalog should have > >>>> been able to absorb it without that spilling over to all the layers. > >>>> > >>>> So for example, if we call dp_ctrl_reset() --> ctrl->catalog->reset_ctrl() > >>>> > >>>> Then the reset_ctrl op of the catalog should manage any controller > >>>> version differences within the reset sequence. > >>> > >>> The problem is that the register-level writes are usually not the best > >>> abstraction. So, instead of designing the code around dp_catalog I'd > >>> prefer to see actual hw / programming changes first. > >>> > >> > >> So thats the issue here. If we did end up with registers and sequences > >> different for controller versions, the ctrl layer was expected to just > >> call a reset() op for example similar to the DPU example you gave. And > >> as you rightly noted, the dpu_hw_xxx files only expose the ops based on > >> version and the upper layers were supposed to just call into the ops > >> without knowing the register level details. Thats pretty much what > >> dp_ctrl tried to do here. We did not want to expose all the register > >> defines in those layers. This series is doing exactly opposite of that. > > > > We don't have the issue up to now, even though we support DP > > controllers since SDM845 up to SM8650 and X1E80100. The SDE driver has > > v200 vs v420 catalog files for PHY programming, the rest of the > > functions are common. So, for me it looks like a preparation for the > > imaginary case that didn't come to existence up to now. > > So, yes. I want to get rid of extra useless indirection and I want to > > expose register sequences in those layers. > > > > Yes because PHY programming is managed in the PHY driver today and does > not go through catalog whereas in SDE driver it does, I do not have any > other concrete example to give you which exists in the current code > where sequence changes across chipset variants for DP controller and > since I certainly cannot discuss how things can evolve moving forward, > as usual, I have to accept it as one of those things which is not used > today. So yes, I guess the register sequencing point changing across > chipset variants, does not have a good example which I can really share. > > But exposing register sequences within the same file, I am not too sure > about that. For example, you can take a look at > dp_catalog_panel_config_hdr in the SDE code OR even > msm_dp_catalog_panel_enable_vsc_sdp in the current upstream code. Why > should this entire sequence be exposed to the dp_panel layer? Why not? The dp_catalog_panel_config_hdr() is a bit tough, we don't implement similar functions currently. For msm_dp_catalog_panel_enable_vsc_sdp() this is also a logical sequence: configure GENERIC0, write the package to GENERIC0, indicate presence of the VSC SDP. I really don't see why this should go to a separate file. > For smaller functions which are one-liners the redirection seems > redundant but when the sequence is bigger like in the examples I gave, > the logical Vs register sequence separation grows. Thats where the > dp_catalog came from. > > > >> > >>>> > >>>> We do not use or have catalog ops today so it looks redundant as we just > >>>> call the dp_catalog APIs directly but that was really the intention. > >>>> > >>>> Another reason which was behind this split but not applicable to current > >>>> upstream driver is that the AUX is part of the PHY driver in upstream > >>>> but in downstream, that remains a part of catalog and as we know the AUX > >>>> component keeps changing with chipsets especially the settings. That was > >>>> the reason of keeping catalog separate and the only place which should > >>>> deal with registers and not the entire DP driver. > >>>> > >>>> The second point seems not applicable to this driver but first point > >>>> still is. I do admit there is re-direction like ctrl->catalog > >>>> instead of just writing it within dp_ctrl itself but the redirection was > >>>> only because ctrl layers were not really meant to deal with the register > >>>> programming. So for example, now with patch 7 of this series every > >>>> register being written to i exposed in dp_ctrl.c and likewise for other > >>>> files. That seems unnecessary. Because if we do end up with some > >>>> variants which need separate registers written, then we will now have to > >>>> end up touching every file as opposed to only touching dp_catalog. > >>> > >>> Yes. I think that it's a bonus, not a problem. We end up touching the > >>> files that are actually changed, so we see what is happening. Quite > >>> frequently register changes are paired with the functionality changes. > >>> > >> > >> Not exactly. Why should dp_ctrl really know that some register offset or > >> some block shift happened for example. It only needs to know when to > >> reset the hardware and not how. Thats the separation getting broken with > >> this. > > > > Yes. And I'm removing that separation very intentionally. If one is > > looking for AUX programming, they should be looking into dp_aux only, > > not dp_aux & dp_catalog. Likewise all audio code should be in > > dp_audio. By using dp_catalog we ended up with a very very very bad > > abstraction of msm_dp_catalog_audio_get_header() / > > msm_dp_catalog_audio_set_header() / enum > > msm_dp_catalog_audio_sdp_type. Just because reads & writes should go > > through the catalog. > > No, I think this is where there is some correction needed. the > get_header() / set_header() was done not because all writes need to go > through catalog but because the audio headers were thought to be written > only one header at a time and we had thought that read-modify-write had > to be done to preserve the bytes. And when we have to do only one header > at a time and because two headers map to one register, catalog had to > end up managing an audio_map. Now, after checking where it came from as > I commented on that patch, this requirement was not a functional one but > was just trying to preserve the pre-silicon validation scripts sequence, > this part of it can be dropped. So no need of get_header() / > set_header() and an audio_map. Now all registers going through catalog > is another thing which we are still discussing here. You've skipped the msm_dp_catalog_audio_sdp_type enum (which was explicitly mentioned). It is an abstraction which in my opinion also isn't required, but it clearly comes from dp_catalog. > > > For dp_panel likewise there is no need to look into some other source > > file to follow the register sequences. It can all be contained within > > dp_panel.c, helping one to understand the code. > > > > > Last, but not least. Code complexity. dp_catalog.c consists of 1340 > > lines, covering different submodules. It is hard to follow it in this > > way. > > > > Its just a question of spreading up the functions all over, not reducing > code complexity. So yes, it reduces the file size of dp_catalog whereas > increases that of others. Code complexity impact due to that is subjective. The main issue is that dp_catalog now contains unrelated sets of functions. That's code complexity. > > >> > >>> For example (a very simple and dumb one), when designing code around > >>> dp_catalog you ended up adding separate _p1 handlers. > >>> Doing that from the data source point of view demands adding a stream_id param. > >>> > >> > >> I have not checked your comment on that series here but if your concern > > > > This is really a bad cadence. I have provided most of the feedback > > almost a week ago. > > > > Yes, was a very tight week trying to enable upstream developers to land > their platforms such as QCS615 by fixing platform specific dpu things > and had the fixes cycle this week too so as a result my own feature took > a bit of a hit this week :( > > >> is stream_id should not be stored in the catalog but just passed, thats > >> fine, we can change it. stream_id as a param is needed anyway because > >> the register programming layer needs to know which offset to use. This > >> series is not mitigating that fact. > > > > No, my concern was that you have been adding separate _p1() functions > > which are a duplicate of _p0() counterparts. When one looks at the > > dp_catalog.c it is logical: there are two different register areas, so > > there are two distinct sets of functions. If one starts looking from > > the dp_panel point of view, it's obvious that there should be a single > > msm_dp_write_stream() function which accepts stream_id and then > > multiplexes it to go to p0 or p1. > > > > Your multiplexing suggestion of adding a msm_dp_read_pn/msm_dp_write_pn > by passing a stream_id can be done even with current dp_catalog intact > as it will help reduce storing the stream_id in the dp_catalog. So its a > valid suggestion and can be implemented even in the current code and not > tied to the fact that register writing is done in dp_catalog or dp_panel. It can. The point was about the implementation logic, not about the possibility. > > >> > >>> In the DPU driver we also have version-related conditionals in the HW > >>> modules rather than pushing all data access to dpu_hw_catalog.c & > >>> counterparts. > >> > >> The dpu_hw_catalog.c and the dp_catalog.c are not the right files to > >> compare with each other. dp_catalog.c should be compared with > >> dpu_hw_xxx.c and as you noted, DPU version dependencies are handled in > >> those files only and not all over the files like what this series is doing. > > > > Not really. dpu_encoder_phys_cmd_init() checks for the core_major_ver. > > Let me see if other files check for the version under the hood. > > > > Well, thats because only cmd mode panel cares about TE. No other files > from what I checked. I've sent a series which refactors feature bits into core_major_ver. Now HW revision is being checked inside dpu_encoder_phys_wb.c, dpu_kms.c and dpu_rm.c. And I didn't refactor SSPP, which would bring similar checks to dpu_plane.c and possibly dpu_vbif.c > > > Also as you wrote, there are multiple dpu_hw_xxx.c files, each > > handling register issues on its own. We don't have a single file which > > keeps all such differences in one place. > > > > Thats because of the way the registers are laid our in the SW interface > document aligns nicely with the file split we have in the DPU even when > the first DPU post happened. > > But I still dont think its a fair comparison. > > If you really had to go deeper into this thought, then even dp_reg.h > should be broken down into smaller headers because the offsets in > dpu_hw_*** files are relevant only to those files but after this change > all DP files must include dp_reg.h even though they will not be using > all of the offsets. Since current code was already doing that, which it > didnt have to as dp_Catalog was the only one writing all registers, this > went unnoticed. Well... I had a thought about reworking DP into using XML files to describe registers. It will make it slightly cleaner and self-documented, but it most likely will be a single file. > > > > Last, but not least, in the DPU driver there are actual differences > > between generations, which require different code paths. In the DP > > driver there are none. > > > >> > >>> I think it's better to make DP driver reflect DPU rather than keeping > >>> a separate wrapper for no particular reason (note, DPU has hardware > >>> abstractions, but on a block level, not on a register level). > >>> > >> > >> Thats the issue here. DPU hardware blocks are arranged according to the > >> sub-blocks both in the software interface document and hence the code > >> matches it file-by-file. DP registers are grouped by clock domains and > >> the file separation we have today does not match that anyway. Hence > >> grouping link registers writes or pixel clock register writes into > >> dp_ctrl is also not correct that way. Let catalog handle that separation > >> internally which it already does. > > > > I'd say, dp_panel, dp_audio and dp_link are already pretty > > self-contained. I was hoping to look at dp_display vs dp_drm later on, > > once the HPD issue gets resolved. Only dp_ctrl is not that logical > > from my point of view. > >
On 12/14/2024 2:05 PM, Dmitry Baryshkov wrote: > On Sat, 14 Dec 2024 at 22:53, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> Hi Dmitry >> >> On 12/12/2024 3:09 PM, Dmitry Baryshkov wrote: >>> On Thu, 12 Dec 2024 at 21:15, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 12/12/2024 12:52 AM, Dmitry Baryshkov wrote: >>>>> On Thu, 12 Dec 2024 at 04:59, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 12/11/2024 3:41 PM, Dmitry Baryshkov wrote: >>>>>>> Having I/O regions inside a msm_dp_catalog_private() results in extra >>>>>>> layers of one-line wrappers for accessing the data. Move I/O region base >>>>>>> and size to the globally visible struct msm_dp_catalog. >>>>>>> >>>>>>> Reviewed-by: Stephen Boyd <swboyd@chromium.org> >>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>>>>> --- >>>>>>> drivers/gpu/drm/msm/dp/dp_catalog.c | 457 +++++++++++++++--------------------- >>>>>>> drivers/gpu/drm/msm/dp/dp_catalog.h | 12 + >>>>>>> 2 files changed, 197 insertions(+), 272 deletions(-) >>>>>>> >>>>>> >>>>>> >>>>>> Fundamentally, the whole point of catalog was that it needs to be the >>>>>> only place where we want to access the registers. Thats how this really >>>>>> started. >>>>>> >>>>>> This pre-dates my time with the DP driver but as I understand thats what >>>>>> it was for. >>>>>> >>>>>> Basically separating out the logical abstraction vs actual register writes . >>>>>> >>>>>> If there are hardware sequence differences within the controller reset >>>>>> OR any other register offsets which moved around, catalog should have >>>>>> been able to absorb it without that spilling over to all the layers. >>>>>> >>>>>> So for example, if we call dp_ctrl_reset() --> ctrl->catalog->reset_ctrl() >>>>>> >>>>>> Then the reset_ctrl op of the catalog should manage any controller >>>>>> version differences within the reset sequence. >>>>> >>>>> The problem is that the register-level writes are usually not the best >>>>> abstraction. So, instead of designing the code around dp_catalog I'd >>>>> prefer to see actual hw / programming changes first. >>>>> >>>> >>>> So thats the issue here. If we did end up with registers and sequences >>>> different for controller versions, the ctrl layer was expected to just >>>> call a reset() op for example similar to the DPU example you gave. And >>>> as you rightly noted, the dpu_hw_xxx files only expose the ops based on >>>> version and the upper layers were supposed to just call into the ops >>>> without knowing the register level details. Thats pretty much what >>>> dp_ctrl tried to do here. We did not want to expose all the register >>>> defines in those layers. This series is doing exactly opposite of that. >>> >>> We don't have the issue up to now, even though we support DP >>> controllers since SDM845 up to SM8650 and X1E80100. The SDE driver has >>> v200 vs v420 catalog files for PHY programming, the rest of the >>> functions are common. So, for me it looks like a preparation for the >>> imaginary case that didn't come to existence up to now. >>> So, yes. I want to get rid of extra useless indirection and I want to >>> expose register sequences in those layers. >>> >> >> Yes because PHY programming is managed in the PHY driver today and does >> not go through catalog whereas in SDE driver it does, I do not have any >> other concrete example to give you which exists in the current code >> where sequence changes across chipset variants for DP controller and >> since I certainly cannot discuss how things can evolve moving forward, >> as usual, I have to accept it as one of those things which is not used >> today. So yes, I guess the register sequencing point changing across >> chipset variants, does not have a good example which I can really share. >> >> But exposing register sequences within the same file, I am not too sure >> about that. For example, you can take a look at >> dp_catalog_panel_config_hdr in the SDE code OR even >> msm_dp_catalog_panel_enable_vsc_sdp in the current upstream code. Why >> should this entire sequence be exposed to the dp_panel layer? > > Why not? The dp_catalog_panel_config_hdr() is a bit tough, we don't > implement similar functions currently. For > msm_dp_catalog_panel_enable_vsc_sdp() this is also a logical sequence: > configure GENERIC0, write the package to GENERIC0, indicate presence > of the VSC SDP. I really don't see why this should go to a separate > file. > We have to plan for hdr for sure. its not an imaginary use-case. msm_dp_catalog_panel_enable_vsc_sdp() does a bunch of things, packing the size, programming the SDP and triggering the update. dp_panel does not need to know all these details. There are many other examples. Take a look at msm_dp_catalog_ctrl_mainlink_ctrl(). from the dp_ctl standpoint, all it needs to know is it needs to program the mainlink_Ctrl() at some point, it really does not need to know the full sequence of doing that. Thats the abstraction getting lost with this. >> For smaller functions which are one-liners the redirection seems >> redundant but when the sequence is bigger like in the examples I gave, >> the logical Vs register sequence separation grows. Thats where the >> dp_catalog came from. >> >> >>>> >>>>>> >>>>>> We do not use or have catalog ops today so it looks redundant as we just >>>>>> call the dp_catalog APIs directly but that was really the intention. >>>>>> >>>>>> Another reason which was behind this split but not applicable to current >>>>>> upstream driver is that the AUX is part of the PHY driver in upstream >>>>>> but in downstream, that remains a part of catalog and as we know the AUX >>>>>> component keeps changing with chipsets especially the settings. That was >>>>>> the reason of keeping catalog separate and the only place which should >>>>>> deal with registers and not the entire DP driver. >>>>>> >>>>>> The second point seems not applicable to this driver but first point >>>>>> still is. I do admit there is re-direction like ctrl->catalog >>>>>> instead of just writing it within dp_ctrl itself but the redirection was >>>>>> only because ctrl layers were not really meant to deal with the register >>>>>> programming. So for example, now with patch 7 of this series every >>>>>> register being written to i exposed in dp_ctrl.c and likewise for other >>>>>> files. That seems unnecessary. Because if we do end up with some >>>>>> variants which need separate registers written, then we will now have to >>>>>> end up touching every file as opposed to only touching dp_catalog. >>>>> >>>>> Yes. I think that it's a bonus, not a problem. We end up touching the >>>>> files that are actually changed, so we see what is happening. Quite >>>>> frequently register changes are paired with the functionality changes. >>>>> >>>> >>>> Not exactly. Why should dp_ctrl really know that some register offset or >>>> some block shift happened for example. It only needs to know when to >>>> reset the hardware and not how. Thats the separation getting broken with >>>> this. >>> >>> Yes. And I'm removing that separation very intentionally. If one is >>> looking for AUX programming, they should be looking into dp_aux only, >>> not dp_aux & dp_catalog. Likewise all audio code should be in >>> dp_audio. By using dp_catalog we ended up with a very very very bad >>> abstraction of msm_dp_catalog_audio_get_header() / >>> msm_dp_catalog_audio_set_header() / enum >>> msm_dp_catalog_audio_sdp_type. Just because reads & writes should go >>> through the catalog. >> >> No, I think this is where there is some correction needed. the >> get_header() / set_header() was done not because all writes need to go >> through catalog but because the audio headers were thought to be written >> only one header at a time and we had thought that read-modify-write had >> to be done to preserve the bytes. And when we have to do only one header >> at a time and because two headers map to one register, catalog had to >> end up managing an audio_map. Now, after checking where it came from as >> I commented on that patch, this requirement was not a functional one but >> was just trying to preserve the pre-silicon validation scripts sequence, >> this part of it can be dropped. So no need of get_header() / >> set_header() and an audio_map. Now all registers going through catalog >> is another thing which we are still discussing here. > > You've skipped the msm_dp_catalog_audio_sdp_type enum (which was > explicitly mentioned). It is an abstraction which in my opinion also > isn't required, but it clearly comes from dp_catalog. > msm_dp_catalog_audio_sdp_type was needed only till the map was needed as it was made with an intention of trying to re-use the sdp_map layout for different packets. So its still tied to the fact that we needed a map. After dropping the map, this can be dropped too as you already did. >> >>> For dp_panel likewise there is no need to look into some other source >>> file to follow the register sequences. It can all be contained within >>> dp_panel.c, helping one to understand the code. >>> >> >>> Last, but not least. Code complexity. dp_catalog.c consists of 1340 >>> lines, covering different submodules. It is hard to follow it in this >>> way. >>> >> >> Its just a question of spreading up the functions all over, not reducing >> code complexity. So yes, it reduces the file size of dp_catalog whereas >> increases that of others. Code complexity impact due to that is subjective. > > The main issue is that dp_catalog now contains unrelated sets of > functions. That's code complexity. > dp_catalog was never meant to be a place where we have related functions. It was supposed to provide the register space abstraction and hiding away the details from rest of the layers. Going by what you are saying even the APIs in dsi_host.c should be related but they are not because of the same reason. I think this series went too far in terms of what it was trying to achieve trying to clenaup even useful things. Audio map removal was a problem tied to the fact that that read-modify-write behavior was preserved. That was removed after we identified its background. No issues with that. but beyond that, this is too much of a rework. I will let other developers chime into this. But I am not too fond of this one. >> >>>> >>>>> For example (a very simple and dumb one), when designing code around >>>>> dp_catalog you ended up adding separate _p1 handlers. >>>>> Doing that from the data source point of view demands adding a stream_id param. >>>>> >>>> >>>> I have not checked your comment on that series here but if your concern >>> >>> This is really a bad cadence. I have provided most of the feedback >>> almost a week ago. >>> >> >> Yes, was a very tight week trying to enable upstream developers to land >> their platforms such as QCS615 by fixing platform specific dpu things >> and had the fixes cycle this week too so as a result my own feature took >> a bit of a hit this week :( >> >>>> is stream_id should not be stored in the catalog but just passed, thats >>>> fine, we can change it. stream_id as a param is needed anyway because >>>> the register programming layer needs to know which offset to use. This >>>> series is not mitigating that fact. >>> >>> No, my concern was that you have been adding separate _p1() functions >>> which are a duplicate of _p0() counterparts. When one looks at the >>> dp_catalog.c it is logical: there are two different register areas, so >>> there are two distinct sets of functions. If one starts looking from >>> the dp_panel point of view, it's obvious that there should be a single >>> msm_dp_write_stream() function which accepts stream_id and then >>> multiplexes it to go to p0 or p1. >>> >> >> Your multiplexing suggestion of adding a msm_dp_read_pn/msm_dp_write_pn >> by passing a stream_id can be done even with current dp_catalog intact >> as it will help reduce storing the stream_id in the dp_catalog. So its a >> valid suggestion and can be implemented even in the current code and not >> tied to the fact that register writing is done in dp_catalog or dp_panel. > > It can. The point was about the implementation logic, not about the possibility. > >> >>>> >>>>> In the DPU driver we also have version-related conditionals in the HW >>>>> modules rather than pushing all data access to dpu_hw_catalog.c & >>>>> counterparts. >>>> >>>> The dpu_hw_catalog.c and the dp_catalog.c are not the right files to >>>> compare with each other. dp_catalog.c should be compared with >>>> dpu_hw_xxx.c and as you noted, DPU version dependencies are handled in >>>> those files only and not all over the files like what this series is doing. >>> >>> Not really. dpu_encoder_phys_cmd_init() checks for the core_major_ver. >>> Let me see if other files check for the version under the hood. >>> >> >> Well, thats because only cmd mode panel cares about TE. No other files >> from what I checked. > > I've sent a series which refactors feature bits into core_major_ver. > Now HW revision is being checked inside dpu_encoder_phys_wb.c, > dpu_kms.c and dpu_rm.c. And I didn't refactor SSPP, which would bring > similar checks to dpu_plane.c and possibly dpu_vbif.c > We will evaluate it on its merits / demerits as usual and decide. >> >>> Also as you wrote, there are multiple dpu_hw_xxx.c files, each >>> handling register issues on its own. We don't have a single file which >>> keeps all such differences in one place. >>> >> >> Thats because of the way the registers are laid our in the SW interface >> document aligns nicely with the file split we have in the DPU even when >> the first DPU post happened. >> >> But I still dont think its a fair comparison. >> >> If you really had to go deeper into this thought, then even dp_reg.h >> should be broken down into smaller headers because the offsets in >> dpu_hw_*** files are relevant only to those files but after this change >> all DP files must include dp_reg.h even though they will not be using >> all of the offsets. Since current code was already doing that, which it >> didnt have to as dp_Catalog was the only one writing all registers, this >> went unnoticed. > > Well... I had a thought about reworking DP into using XML files to > describe registers. It will make it slightly cleaner and > self-documented, but it most likely will be a single file. > It being in a single file is leaning towards the same model we have right now. If dp_regs.h is remaining one unified file, I dont see why dp_catalog can stay as well. >> >> >>> Last, but not least, in the DPU driver there are actual differences >>> between generations, which require different code paths. In the DP >>> driver there are none. >>> >>>> >>>>> I think it's better to make DP driver reflect DPU rather than keeping >>>>> a separate wrapper for no particular reason (note, DPU has hardware >>>>> abstractions, but on a block level, not on a register level). >>>>> >>>> >>>> Thats the issue here. DPU hardware blocks are arranged according to the >>>> sub-blocks both in the software interface document and hence the code >>>> matches it file-by-file. DP registers are grouped by clock domains and >>>> the file separation we have today does not match that anyway. Hence >>>> grouping link registers writes or pixel clock register writes into >>>> dp_ctrl is also not correct that way. Let catalog handle that separation >>>> internally which it already does. >>> >>> I'd say, dp_panel, dp_audio and dp_link are already pretty >>> self-contained. I was hoping to look at dp_display vs dp_drm later on, >>> once the HPD issue gets resolved. Only dp_ctrl is not that logical >>> from my point of view. >>> Not from the point of view of the register separation of what belongs where. Its just a subjective opinion of what belongs where. Different developers can view it differently. > > >
On Mon, Dec 16, 2024 at 12:45:13PM -0800, Abhinav Kumar wrote: > > > On 12/14/2024 2:05 PM, Dmitry Baryshkov wrote: > > On Sat, 14 Dec 2024 at 22:53, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > > > Hi Dmitry > > > > > > On 12/12/2024 3:09 PM, Dmitry Baryshkov wrote: > > > > On Thu, 12 Dec 2024 at 21:15, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > > > > > > > > > > > > > > > > > On 12/12/2024 12:52 AM, Dmitry Baryshkov wrote: > > > > > > On Thu, 12 Dec 2024 at 04:59, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 12/11/2024 3:41 PM, Dmitry Baryshkov wrote: > > > > > > > > Having I/O regions inside a msm_dp_catalog_private() results in extra > > > > > > > > layers of one-line wrappers for accessing the data. Move I/O region base > > > > > > > > and size to the globally visible struct msm_dp_catalog. > > > > > > > > > > > > > > > > Reviewed-by: Stephen Boyd <swboyd@chromium.org> > > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > > > > --- > > > > > > > > drivers/gpu/drm/msm/dp/dp_catalog.c | 457 +++++++++++++++--------------------- > > > > > > > > drivers/gpu/drm/msm/dp/dp_catalog.h | 12 + > > > > > > > > 2 files changed, 197 insertions(+), 272 deletions(-) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fundamentally, the whole point of catalog was that it needs to be the > > > > > > > only place where we want to access the registers. Thats how this really > > > > > > > started. > > > > > > > > > > > > > > This pre-dates my time with the DP driver but as I understand thats what > > > > > > > it was for. > > > > > > > > > > > > > > Basically separating out the logical abstraction vs actual register writes . > > > > > > > > > > > > > > If there are hardware sequence differences within the controller reset > > > > > > > OR any other register offsets which moved around, catalog should have > > > > > > > been able to absorb it without that spilling over to all the layers. > > > > > > > > > > > > > > So for example, if we call dp_ctrl_reset() --> ctrl->catalog->reset_ctrl() > > > > > > > > > > > > > > Then the reset_ctrl op of the catalog should manage any controller > > > > > > > version differences within the reset sequence. > > > > > > > > > > > > The problem is that the register-level writes are usually not the best > > > > > > abstraction. So, instead of designing the code around dp_catalog I'd > > > > > > prefer to see actual hw / programming changes first. > > > > > > > > > > > > > > > > So thats the issue here. If we did end up with registers and sequences > > > > > different for controller versions, the ctrl layer was expected to just > > > > > call a reset() op for example similar to the DPU example you gave. And > > > > > as you rightly noted, the dpu_hw_xxx files only expose the ops based on > > > > > version and the upper layers were supposed to just call into the ops > > > > > without knowing the register level details. Thats pretty much what > > > > > dp_ctrl tried to do here. We did not want to expose all the register > > > > > defines in those layers. This series is doing exactly opposite of that. > > > > > > > > We don't have the issue up to now, even though we support DP > > > > controllers since SDM845 up to SM8650 and X1E80100. The SDE driver has > > > > v200 vs v420 catalog files for PHY programming, the rest of the > > > > functions are common. So, for me it looks like a preparation for the > > > > imaginary case that didn't come to existence up to now. > > > > So, yes. I want to get rid of extra useless indirection and I want to > > > > expose register sequences in those layers. > > > > > > > > > > Yes because PHY programming is managed in the PHY driver today and does > > > not go through catalog whereas in SDE driver it does, I do not have any > > > other concrete example to give you which exists in the current code > > > where sequence changes across chipset variants for DP controller and > > > since I certainly cannot discuss how things can evolve moving forward, > > > as usual, I have to accept it as one of those things which is not used > > > today. So yes, I guess the register sequencing point changing across > > > chipset variants, does not have a good example which I can really share. > > > > > > But exposing register sequences within the same file, I am not too sure > > > about that. For example, you can take a look at > > > dp_catalog_panel_config_hdr in the SDE code OR even > > > msm_dp_catalog_panel_enable_vsc_sdp in the current upstream code. Why > > > should this entire sequence be exposed to the dp_panel layer? > > > > Why not? The dp_catalog_panel_config_hdr() is a bit tough, we don't > > implement similar functions currently. For > > msm_dp_catalog_panel_enable_vsc_sdp() this is also a logical sequence: > > configure GENERIC0, write the package to GENERIC0, indicate presence > > of the VSC SDP. I really don't see why this should go to a separate > > file. > > > > We have to plan for hdr for sure. its not an imaginary use-case. > > msm_dp_catalog_panel_enable_vsc_sdp() does a bunch of things, packing the > size, programming the SDP and triggering the update. dp_panel does not need > to know all these details. > > There are many other examples. Take a look at > msm_dp_catalog_ctrl_mainlink_ctrl(). from the dp_ctl standpoint, all it > needs to know is it needs to program the mainlink_Ctrl() at some point, it > really does not need to know the full sequence of doing that. Thats the > abstraction getting lost with this. > > > > For smaller functions which are one-liners the redirection seems > > > redundant but when the sequence is bigger like in the examples I gave, > > > the logical Vs register sequence separation grows. Thats where the > > > dp_catalog came from. > > > > > > > > > > > > > > > > > > > > > > > > > We do not use or have catalog ops today so it looks redundant as we just > > > > > > > call the dp_catalog APIs directly but that was really the intention. > > > > > > > > > > > > > > Another reason which was behind this split but not applicable to current > > > > > > > upstream driver is that the AUX is part of the PHY driver in upstream > > > > > > > but in downstream, that remains a part of catalog and as we know the AUX > > > > > > > component keeps changing with chipsets especially the settings. That was > > > > > > > the reason of keeping catalog separate and the only place which should > > > > > > > deal with registers and not the entire DP driver. > > > > > > > > > > > > > > The second point seems not applicable to this driver but first point > > > > > > > still is. I do admit there is re-direction like ctrl->catalog > > > > > > > instead of just writing it within dp_ctrl itself but the redirection was > > > > > > > only because ctrl layers were not really meant to deal with the register > > > > > > > programming. So for example, now with patch 7 of this series every > > > > > > > register being written to i exposed in dp_ctrl.c and likewise for other > > > > > > > files. That seems unnecessary. Because if we do end up with some > > > > > > > variants which need separate registers written, then we will now have to > > > > > > > end up touching every file as opposed to only touching dp_catalog. > > > > > > > > > > > > Yes. I think that it's a bonus, not a problem. We end up touching the > > > > > > files that are actually changed, so we see what is happening. Quite > > > > > > frequently register changes are paired with the functionality changes. > > > > > > > > > > > > > > > > Not exactly. Why should dp_ctrl really know that some register offset or > > > > > some block shift happened for example. It only needs to know when to > > > > > reset the hardware and not how. Thats the separation getting broken with > > > > > this. > > > > > > > > Yes. And I'm removing that separation very intentionally. If one is > > > > looking for AUX programming, they should be looking into dp_aux only, > > > > not dp_aux & dp_catalog. Likewise all audio code should be in > > > > dp_audio. By using dp_catalog we ended up with a very very very bad > > > > abstraction of msm_dp_catalog_audio_get_header() / > > > > msm_dp_catalog_audio_set_header() / enum > > > > msm_dp_catalog_audio_sdp_type. Just because reads & writes should go > > > > through the catalog. > > > > > > No, I think this is where there is some correction needed. the > > > get_header() / set_header() was done not because all writes need to go > > > through catalog but because the audio headers were thought to be written > > > only one header at a time and we had thought that read-modify-write had > > > to be done to preserve the bytes. And when we have to do only one header > > > at a time and because two headers map to one register, catalog had to > > > end up managing an audio_map. Now, after checking where it came from as > > > I commented on that patch, this requirement was not a functional one but > > > was just trying to preserve the pre-silicon validation scripts sequence, > > > this part of it can be dropped. So no need of get_header() / > > > set_header() and an audio_map. Now all registers going through catalog > > > is another thing which we are still discussing here. > > > > You've skipped the msm_dp_catalog_audio_sdp_type enum (which was > > explicitly mentioned). It is an abstraction which in my opinion also > > isn't required, but it clearly comes from dp_catalog. > > > > msm_dp_catalog_audio_sdp_type was needed only till the map was needed as it > was made with an intention of trying to re-use the sdp_map layout for > different packets. So its still tied to the fact that we needed a map. After > dropping the map, this can be dropped too as you already did. I'm not sure if I understood what you've meant by the sdp_map layout re-use. > > > > > > > > > For dp_panel likewise there is no need to look into some other source > > > > file to follow the register sequences. It can all be contained within > > > > dp_panel.c, helping one to understand the code. > > > > > > > > > > > Last, but not least. Code complexity. dp_catalog.c consists of 1340 > > > > lines, covering different submodules. It is hard to follow it in this > > > > way. > > > > > > > > > > Its just a question of spreading up the functions all over, not reducing > > > code complexity. So yes, it reduces the file size of dp_catalog whereas > > > increases that of others. Code complexity impact due to that is subjective. > > > > The main issue is that dp_catalog now contains unrelated sets of > > functions. That's code complexity. > > > > dp_catalog was never meant to be a place where we have related functions. It > was supposed to provide the register space abstraction and hiding away the > details from rest of the layers. Going by what you are saying even the APIs > in dsi_host.c should be related but they are not because of the same reason. > I think this series went too far in terms of what it was trying to achieve > trying to clenaup even useful things. Audio map removal was a problem tied > to the fact that that read-modify-write behavior was preserved. That was > removed after we identified its background. No issues with that. but beyond > that, this is too much of a rework. I will let other developers chime into > this. But I am not too fond of this one. Sure. It seems the important parts have been reviewed in the next iteration, so we can land those. Stephen, one of DP reviewers / expers, seems to like the remaining patches. > > > > > > > > > > > > > > > For example (a very simple and dumb one), when designing code around > > > > > > dp_catalog you ended up adding separate _p1 handlers. > > > > > > Doing that from the data source point of view demands adding a stream_id param. > > > > > > > > > > > > > > > > I have not checked your comment on that series here but if your concern > > > > > > > > This is really a bad cadence. I have provided most of the feedback > > > > almost a week ago. > > > > > > > > > > Yes, was a very tight week trying to enable upstream developers to land > > > their platforms such as QCS615 by fixing platform specific dpu things > > > and had the fixes cycle this week too so as a result my own feature took > > > a bit of a hit this week :( > > > > > > > > is stream_id should not be stored in the catalog but just passed, thats > > > > > fine, we can change it. stream_id as a param is needed anyway because > > > > > the register programming layer needs to know which offset to use. This > > > > > series is not mitigating that fact. > > > > > > > > No, my concern was that you have been adding separate _p1() functions > > > > which are a duplicate of _p0() counterparts. When one looks at the > > > > dp_catalog.c it is logical: there are two different register areas, so > > > > there are two distinct sets of functions. If one starts looking from > > > > the dp_panel point of view, it's obvious that there should be a single > > > > msm_dp_write_stream() function which accepts stream_id and then > > > > multiplexes it to go to p0 or p1. > > > > > > > > > > Your multiplexing suggestion of adding a msm_dp_read_pn/msm_dp_write_pn > > > by passing a stream_id can be done even with current dp_catalog intact > > > as it will help reduce storing the stream_id in the dp_catalog. So its a > > > valid suggestion and can be implemented even in the current code and not > > > tied to the fact that register writing is done in dp_catalog or dp_panel. > > > > It can. The point was about the implementation logic, not about the possibility. > > > > > > > > > > > > > > > > In the DPU driver we also have version-related conditionals in the HW > > > > > > modules rather than pushing all data access to dpu_hw_catalog.c & > > > > > > counterparts. > > > > > > > > > > The dpu_hw_catalog.c and the dp_catalog.c are not the right files to > > > > > compare with each other. dp_catalog.c should be compared with > > > > > dpu_hw_xxx.c and as you noted, DPU version dependencies are handled in > > > > > those files only and not all over the files like what this series is doing. > > > > > > > > Not really. dpu_encoder_phys_cmd_init() checks for the core_major_ver. > > > > Let me see if other files check for the version under the hood. > > > > > > > > > > Well, thats because only cmd mode panel cares about TE. No other files > > > from what I checked. > > > > I've sent a series which refactors feature bits into core_major_ver. > > Now HW revision is being checked inside dpu_encoder_phys_wb.c, > > dpu_kms.c and dpu_rm.c. And I didn't refactor SSPP, which would bring > > similar checks to dpu_plane.c and possibly dpu_vbif.c > > > > We will evaluate it on its merits / demerits as usual and decide. Sure :-) > > > > > > > > Also as you wrote, there are multiple dpu_hw_xxx.c files, each > > > > handling register issues on its own. We don't have a single file which > > > > keeps all such differences in one place. > > > > > > > > > > Thats because of the way the registers are laid our in the SW interface > > > document aligns nicely with the file split we have in the DPU even when > > > the first DPU post happened. > > > > > > But I still dont think its a fair comparison. > > > > > > If you really had to go deeper into this thought, then even dp_reg.h > > > should be broken down into smaller headers because the offsets in > > > dpu_hw_*** files are relevant only to those files but after this change > > > all DP files must include dp_reg.h even though they will not be using > > > all of the offsets. Since current code was already doing that, which it > > > didnt have to as dp_Catalog was the only one writing all registers, this > > > went unnoticed. > > > > Well... I had a thought about reworking DP into using XML files to > > describe registers. It will make it slightly cleaner and > > self-documented, but it most likely will be a single file. > > > > It being in a single file is leaning towards the same model we have right > now. If dp_regs.h is remaining one unified file, I dont see why dp_catalog > can stay as well. It is typical to have a single file with the full register information. MDP5 for example also has a single register file. > > > > > > > > > > > Last, but not least, in the DPU driver there are actual differences > > > > between generations, which require different code paths. In the DP > > > > driver there are none. > > > > > > > > > > > > > > > I think it's better to make DP driver reflect DPU rather than keeping > > > > > > a separate wrapper for no particular reason (note, DPU has hardware > > > > > > abstractions, but on a block level, not on a register level). > > > > > > > > > > > > > > > > Thats the issue here. DPU hardware blocks are arranged according to the > > > > > sub-blocks both in the software interface document and hence the code > > > > > matches it file-by-file. DP registers are grouped by clock domains and > > > > > the file separation we have today does not match that anyway. Hence > > > > > grouping link registers writes or pixel clock register writes into > > > > > dp_ctrl is also not correct that way. Let catalog handle that separation > > > > > internally which it already does. > > > > > > > > I'd say, dp_panel, dp_audio and dp_link are already pretty > > > > self-contained. I was hoping to look at dp_display vs dp_drm later on, > > > > once the HPD issue gets resolved. Only dp_ctrl is not that logical > > > > from my point of view. > > > > > > Not from the point of view of the register separation of what belongs where. > Its just a subjective opinion of what belongs where. Different developers > can view it differently. Even from the register separation point of view: it's better to separate the AUX and p0/p1 functions rather than keeping them in the dp_catalog.c together with the rest of the unrelated functions. Note, that dp_link.c also uses dp_reg.h though it doesn't use dp_catalog at all.
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index 0357dec1acd5773f25707e7ebdfca4b1d2b1bb4e..cdb8685924a06e4fc79d70586630ccb9a16a676d 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -63,155 +63,128 @@ #define DP_DEFAULT_P0_OFFSET 0x1000 #define DP_DEFAULT_P0_SIZE 0x0400 -struct dss_io_region { - size_t len; - void __iomem *base; -}; - -struct dss_io_data { - struct dss_io_region ahb; - struct dss_io_region aux; - struct dss_io_region link; - struct dss_io_region p0; -}; - struct msm_dp_catalog_private { struct device *dev; struct drm_device *drm_dev; - struct dss_io_data io; u32 (*audio_map)[DP_AUDIO_SDP_HEADER_MAX]; struct msm_dp_catalog msm_dp_catalog; }; void msm_dp_catalog_snapshot(struct msm_dp_catalog *msm_dp_catalog, struct msm_disp_state *disp_state) { - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); - struct dss_io_data *dss = &catalog->io; - - msm_disp_snapshot_add_block(disp_state, dss->ahb.len, dss->ahb.base, "dp_ahb"); - msm_disp_snapshot_add_block(disp_state, dss->aux.len, dss->aux.base, "dp_aux"); - msm_disp_snapshot_add_block(disp_state, dss->link.len, dss->link.base, "dp_link"); - msm_disp_snapshot_add_block(disp_state, dss->p0.len, dss->p0.base, "dp_p0"); + msm_disp_snapshot_add_block(disp_state, + msm_dp_catalog->ahb_len, msm_dp_catalog->ahb_base, "dp_ahb"); + msm_disp_snapshot_add_block(disp_state, + msm_dp_catalog->aux_len, msm_dp_catalog->aux_base, "dp_aux"); + msm_disp_snapshot_add_block(disp_state, + msm_dp_catalog->link_len, msm_dp_catalog->link_base, "dp_link"); + msm_disp_snapshot_add_block(disp_state, + msm_dp_catalog->p0_len, msm_dp_catalog->p0_base, "dp_p0"); } -static inline u32 msm_dp_read_aux(struct msm_dp_catalog_private *catalog, u32 offset) +static inline u32 msm_dp_read_aux(struct msm_dp_catalog *msm_dp_catalog, u32 offset) { - return readl_relaxed(catalog->io.aux.base + offset); + return readl_relaxed(msm_dp_catalog->aux_base + offset); } -static inline void msm_dp_write_aux(struct msm_dp_catalog_private *catalog, +static inline void msm_dp_write_aux(struct msm_dp_catalog *msm_dp_catalog, u32 offset, u32 data) { /* * To make sure aux reg writes happens before any other operation, * this function uses writel() instread of writel_relaxed() */ - writel(data, catalog->io.aux.base + offset); + writel(data, msm_dp_catalog->aux_base + offset); } -static inline u32 msm_dp_read_ahb(const struct msm_dp_catalog_private *catalog, u32 offset) +static inline u32 msm_dp_read_ahb(const struct msm_dp_catalog *msm_dp_catalog, u32 offset) { - return readl_relaxed(catalog->io.ahb.base + offset); + return readl_relaxed(msm_dp_catalog->ahb_base + offset); } -static inline void msm_dp_write_ahb(struct msm_dp_catalog_private *catalog, +static inline void msm_dp_write_ahb(struct msm_dp_catalog *msm_dp_catalog, u32 offset, u32 data) { /* * To make sure phy reg writes happens before any other operation, * this function uses writel() instread of writel_relaxed() */ - writel(data, catalog->io.ahb.base + offset); + writel(data, msm_dp_catalog->ahb_base + offset); } -static inline void msm_dp_write_p0(struct msm_dp_catalog_private *catalog, +static inline void msm_dp_write_p0(struct msm_dp_catalog *msm_dp_catalog, u32 offset, u32 data) { /* * To make sure interface reg writes happens before any other operation, * this function uses writel() instread of writel_relaxed() */ - writel(data, catalog->io.p0.base + offset); + writel(data, msm_dp_catalog->p0_base + offset); } -static inline u32 msm_dp_read_p0(struct msm_dp_catalog_private *catalog, +static inline u32 msm_dp_read_p0(struct msm_dp_catalog *msm_dp_catalog, u32 offset) { /* * To make sure interface reg writes happens before any other operation, * this function uses writel() instread of writel_relaxed() */ - return readl_relaxed(catalog->io.p0.base + offset); + return readl_relaxed(msm_dp_catalog->p0_base + offset); } -static inline u32 msm_dp_read_link(struct msm_dp_catalog_private *catalog, u32 offset) +static inline u32 msm_dp_read_link(struct msm_dp_catalog *msm_dp_catalog, u32 offset) { - return readl_relaxed(catalog->io.link.base + offset); + return readl_relaxed(msm_dp_catalog->link_base + offset); } -static inline void msm_dp_write_link(struct msm_dp_catalog_private *catalog, +static inline void msm_dp_write_link(struct msm_dp_catalog *msm_dp_catalog, u32 offset, u32 data) { /* * To make sure link reg writes happens before any other operation, * this function uses writel() instread of writel_relaxed() */ - writel(data, catalog->io.link.base + offset); + writel(data, msm_dp_catalog->link_base + offset); } /* aux related catalog functions */ u32 msm_dp_catalog_aux_read_data(struct msm_dp_catalog *msm_dp_catalog) { - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); - - return msm_dp_read_aux(catalog, REG_DP_AUX_DATA); + return msm_dp_read_aux(msm_dp_catalog, REG_DP_AUX_DATA); } int msm_dp_catalog_aux_write_data(struct msm_dp_catalog *msm_dp_catalog, u32 data) { - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); - - msm_dp_write_aux(catalog, REG_DP_AUX_DATA, data); + msm_dp_write_aux(msm_dp_catalog, REG_DP_AUX_DATA, data); return 0; } int msm_dp_catalog_aux_write_trans(struct msm_dp_catalog *msm_dp_catalog, u32 data) { - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); - - msm_dp_write_aux(catalog, REG_DP_AUX_TRANS_CTRL, data); + msm_dp_write_aux(msm_dp_catalog, REG_DP_AUX_TRANS_CTRL, data); return 0; } int msm_dp_catalog_aux_clear_trans(struct msm_dp_catalog *msm_dp_catalog, bool read) { u32 data; - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); if (read) { - data = msm_dp_read_aux(catalog, REG_DP_AUX_TRANS_CTRL); + data = msm_dp_read_aux(msm_dp_catalog, REG_DP_AUX_TRANS_CTRL); data &= ~DP_AUX_TRANS_CTRL_GO; - msm_dp_write_aux(catalog, REG_DP_AUX_TRANS_CTRL, data); + msm_dp_write_aux(msm_dp_catalog, REG_DP_AUX_TRANS_CTRL, data); } else { - msm_dp_write_aux(catalog, REG_DP_AUX_TRANS_CTRL, 0); + msm_dp_write_aux(msm_dp_catalog, REG_DP_AUX_TRANS_CTRL, 0); } return 0; } int msm_dp_catalog_aux_clear_hw_interrupts(struct msm_dp_catalog *msm_dp_catalog) { - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); - - msm_dp_read_aux(catalog, REG_DP_PHY_AUX_INTERRUPT_STATUS); - msm_dp_write_aux(catalog, REG_DP_PHY_AUX_INTERRUPT_CLEAR, 0x1f); - msm_dp_write_aux(catalog, REG_DP_PHY_AUX_INTERRUPT_CLEAR, 0x9f); - msm_dp_write_aux(catalog, REG_DP_PHY_AUX_INTERRUPT_CLEAR, 0); + msm_dp_read_aux(msm_dp_catalog, REG_DP_PHY_AUX_INTERRUPT_STATUS); + msm_dp_write_aux(msm_dp_catalog, REG_DP_PHY_AUX_INTERRUPT_CLEAR, 0x1f); + msm_dp_write_aux(msm_dp_catalog, REG_DP_PHY_AUX_INTERRUPT_CLEAR, 0x9f); + msm_dp_write_aux(msm_dp_catalog, REG_DP_PHY_AUX_INTERRUPT_CLEAR, 0); return 0; } @@ -230,47 +203,41 @@ int msm_dp_catalog_aux_clear_hw_interrupts(struct msm_dp_catalog *msm_dp_catalog void msm_dp_catalog_aux_reset(struct msm_dp_catalog *msm_dp_catalog) { u32 aux_ctrl; - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); - aux_ctrl = msm_dp_read_aux(catalog, REG_DP_AUX_CTRL); + aux_ctrl = msm_dp_read_aux(msm_dp_catalog, REG_DP_AUX_CTRL); aux_ctrl |= DP_AUX_CTRL_RESET; - msm_dp_write_aux(catalog, REG_DP_AUX_CTRL, aux_ctrl); + msm_dp_write_aux(msm_dp_catalog, REG_DP_AUX_CTRL, aux_ctrl); usleep_range(1000, 1100); /* h/w recommended delay */ aux_ctrl &= ~DP_AUX_CTRL_RESET; - msm_dp_write_aux(catalog, REG_DP_AUX_CTRL, aux_ctrl); + msm_dp_write_aux(msm_dp_catalog, REG_DP_AUX_CTRL, aux_ctrl); } void msm_dp_catalog_aux_enable(struct msm_dp_catalog *msm_dp_catalog, bool enable) { u32 aux_ctrl; - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); - aux_ctrl = msm_dp_read_aux(catalog, REG_DP_AUX_CTRL); + aux_ctrl = msm_dp_read_aux(msm_dp_catalog, REG_DP_AUX_CTRL); if (enable) { - msm_dp_write_aux(catalog, REG_DP_TIMEOUT_COUNT, 0xffff); - msm_dp_write_aux(catalog, REG_DP_AUX_LIMITS, 0xffff); + msm_dp_write_aux(msm_dp_catalog, REG_DP_TIMEOUT_COUNT, 0xffff); + msm_dp_write_aux(msm_dp_catalog, REG_DP_AUX_LIMITS, 0xffff); aux_ctrl |= DP_AUX_CTRL_ENABLE; } else { aux_ctrl &= ~DP_AUX_CTRL_ENABLE; } - msm_dp_write_aux(catalog, REG_DP_AUX_CTRL, aux_ctrl); + msm_dp_write_aux(msm_dp_catalog, REG_DP_AUX_CTRL, aux_ctrl); } int msm_dp_catalog_aux_wait_for_hpd_connect_state(struct msm_dp_catalog *msm_dp_catalog, unsigned long wait_us) { u32 state; - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); /* poll for hpd connected status every 2ms and timeout after wait_us */ - return readl_poll_timeout(catalog->io.aux.base + + return readl_poll_timeout(msm_dp_catalog->aux_base + REG_DP_DP_HPD_INT_STATUS, state, state & DP_DP_HPD_STATE_STATUS_CONNECTED, min(wait_us, 2000), wait_us); @@ -278,15 +245,13 @@ int msm_dp_catalog_aux_wait_for_hpd_connect_state(struct msm_dp_catalog *msm_dp_ u32 msm_dp_catalog_aux_get_irq(struct msm_dp_catalog *msm_dp_catalog) { - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); u32 intr, intr_ack; - intr = msm_dp_read_ahb(catalog, REG_DP_INTR_STATUS); + intr = msm_dp_read_ahb(msm_dp_catalog, REG_DP_INTR_STATUS); intr &= ~DP_INTERRUPT_STATUS1_MASK; intr_ack = (intr & DP_INTERRUPT_STATUS1) << DP_INTERRUPT_STATUS_ACK_SHIFT; - msm_dp_write_ahb(catalog, REG_DP_INTR_STATUS, intr_ack | + msm_dp_write_ahb(msm_dp_catalog, REG_DP_INTR_STATUS, intr_ack | DP_INTERRUPT_STATUS1_MASK); return intr; @@ -298,20 +263,14 @@ void msm_dp_catalog_ctrl_update_transfer_unit(struct msm_dp_catalog *msm_dp_cata u32 msm_dp_tu, u32 valid_boundary, u32 valid_boundary2) { - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); - - msm_dp_write_link(catalog, REG_DP_VALID_BOUNDARY, valid_boundary); - msm_dp_write_link(catalog, REG_DP_TU, msm_dp_tu); - msm_dp_write_link(catalog, REG_DP_VALID_BOUNDARY_2, valid_boundary2); + msm_dp_write_link(msm_dp_catalog, REG_DP_VALID_BOUNDARY, valid_boundary); + msm_dp_write_link(msm_dp_catalog, REG_DP_TU, msm_dp_tu); + msm_dp_write_link(msm_dp_catalog, REG_DP_VALID_BOUNDARY_2, valid_boundary2); } void msm_dp_catalog_ctrl_state_ctrl(struct msm_dp_catalog *msm_dp_catalog, u32 state) { - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); - - msm_dp_write_link(catalog, REG_DP_STATE_CTRL, state); + msm_dp_write_link(msm_dp_catalog, REG_DP_STATE_CTRL, state); } void msm_dp_catalog_ctrl_config_ctrl(struct msm_dp_catalog *msm_dp_catalog, u32 cfg) @@ -321,13 +280,11 @@ void msm_dp_catalog_ctrl_config_ctrl(struct msm_dp_catalog *msm_dp_catalog, u32 drm_dbg_dp(catalog->drm_dev, "DP_CONFIGURATION_CTRL=0x%x\n", cfg); - msm_dp_write_link(catalog, REG_DP_CONFIGURATION_CTRL, cfg); + msm_dp_write_link(msm_dp_catalog, REG_DP_CONFIGURATION_CTRL, cfg); } void msm_dp_catalog_ctrl_lane_mapping(struct msm_dp_catalog *msm_dp_catalog) { - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); u32 ln_0 = 0, ln_1 = 1, ln_2 = 2, ln_3 = 3; /* One-to-One mapping */ u32 ln_mapping; @@ -336,7 +293,7 @@ void msm_dp_catalog_ctrl_lane_mapping(struct msm_dp_catalog *msm_dp_catalog) ln_mapping |= ln_2 << LANE2_MAPPING_SHIFT; ln_mapping |= ln_3 << LANE3_MAPPING_SHIFT; - msm_dp_write_link(catalog, REG_DP_LOGICAL2PHYSICAL_LANE_MAPPING, + msm_dp_write_link(msm_dp_catalog, REG_DP_LOGICAL2PHYSICAL_LANE_MAPPING, ln_mapping); } @@ -344,17 +301,15 @@ void msm_dp_catalog_ctrl_psr_mainlink_enable(struct msm_dp_catalog *msm_dp_catal bool enable) { u32 val; - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); - val = msm_dp_read_link(catalog, REG_DP_MAINLINK_CTRL); + val = msm_dp_read_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL); if (enable) val |= DP_MAINLINK_CTRL_ENABLE; else val &= ~DP_MAINLINK_CTRL_ENABLE; - msm_dp_write_link(catalog, REG_DP_MAINLINK_CTRL, val); + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL, val); } void msm_dp_catalog_ctrl_mainlink_ctrl(struct msm_dp_catalog *msm_dp_catalog, @@ -370,25 +325,25 @@ void msm_dp_catalog_ctrl_mainlink_ctrl(struct msm_dp_catalog *msm_dp_catalog, * To make sure link reg writes happens before other operation, * msm_dp_write_link() function uses writel() */ - mainlink_ctrl = msm_dp_read_link(catalog, REG_DP_MAINLINK_CTRL); + mainlink_ctrl = msm_dp_read_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL); mainlink_ctrl &= ~(DP_MAINLINK_CTRL_RESET | DP_MAINLINK_CTRL_ENABLE); - msm_dp_write_link(catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); mainlink_ctrl |= DP_MAINLINK_CTRL_RESET; - msm_dp_write_link(catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); mainlink_ctrl &= ~DP_MAINLINK_CTRL_RESET; - msm_dp_write_link(catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); mainlink_ctrl |= (DP_MAINLINK_CTRL_ENABLE | DP_MAINLINK_FB_BOUNDARY_SEL); - msm_dp_write_link(catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); } else { - mainlink_ctrl = msm_dp_read_link(catalog, REG_DP_MAINLINK_CTRL); + mainlink_ctrl = msm_dp_read_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL); mainlink_ctrl &= ~DP_MAINLINK_CTRL_ENABLE; - msm_dp_write_link(catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); } } @@ -400,7 +355,7 @@ void msm_dp_catalog_ctrl_config_misc(struct msm_dp_catalog *msm_dp_catalog, struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, struct msm_dp_catalog_private, msm_dp_catalog); - misc_val = msm_dp_read_link(catalog, REG_DP_MISC1_MISC0); + misc_val = msm_dp_read_link(msm_dp_catalog, REG_DP_MISC1_MISC0); /* clear bpp bits */ misc_val &= ~(0x07 << DP_MISC0_TEST_BITS_DEPTH_SHIFT); @@ -410,16 +365,14 @@ void msm_dp_catalog_ctrl_config_misc(struct msm_dp_catalog *msm_dp_catalog, misc_val |= DP_MISC0_SYNCHRONOUS_CLK; drm_dbg_dp(catalog->drm_dev, "misc settings = 0x%x\n", misc_val); - msm_dp_write_link(catalog, REG_DP_MISC1_MISC0, misc_val); + msm_dp_write_link(msm_dp_catalog, REG_DP_MISC1_MISC0, misc_val); } void msm_dp_catalog_setup_peripheral_flush(struct msm_dp_catalog *msm_dp_catalog) { u32 mainlink_ctrl, hw_revision; - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); - mainlink_ctrl = msm_dp_read_link(catalog, REG_DP_MAINLINK_CTRL); + mainlink_ctrl = msm_dp_read_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL); hw_revision = msm_dp_catalog_hw_revision(msm_dp_catalog); if (hw_revision >= DP_HW_VERSION_1_2) @@ -427,7 +380,7 @@ void msm_dp_catalog_setup_peripheral_flush(struct msm_dp_catalog *msm_dp_catalog else mainlink_ctrl |= DP_MAINLINK_FLUSH_MODE_UPDATE_SDP; - msm_dp_write_link(catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); } void msm_dp_catalog_ctrl_config_msa(struct msm_dp_catalog *msm_dp_catalog, @@ -485,9 +438,9 @@ void msm_dp_catalog_ctrl_config_msa(struct msm_dp_catalog *msm_dp_catalog, nvid *= 3; drm_dbg_dp(catalog->drm_dev, "mvid=0x%x, nvid=0x%x\n", mvid, nvid); - msm_dp_write_link(catalog, REG_DP_SOFTWARE_MVID, mvid); - msm_dp_write_link(catalog, REG_DP_SOFTWARE_NVID, nvid); - msm_dp_write_p0(catalog, MMSS_DP_DSC_DTO, 0x0); + msm_dp_write_link(msm_dp_catalog, REG_DP_SOFTWARE_MVID, mvid); + msm_dp_write_link(msm_dp_catalog, REG_DP_SOFTWARE_NVID, nvid); + msm_dp_write_p0(msm_dp_catalog, MMSS_DP_DSC_DTO, 0x0); } int msm_dp_catalog_ctrl_set_pattern_state_bit(struct msm_dp_catalog *msm_dp_catalog, @@ -505,7 +458,7 @@ int msm_dp_catalog_ctrl_set_pattern_state_bit(struct msm_dp_catalog *msm_dp_cata bit = BIT(state_bit - 1) << DP_MAINLINK_READY_LINK_TRAINING_SHIFT; /* Poll for mainlink ready status */ - ret = readx_poll_timeout(readl, catalog->io.link.base + + ret = readx_poll_timeout(readl, msm_dp_catalog->link_base + REG_DP_MAINLINK_READY, data, data & bit, POLLING_SLEEP_US, POLLING_TIMEOUT_US); @@ -526,10 +479,7 @@ int msm_dp_catalog_ctrl_set_pattern_state_bit(struct msm_dp_catalog *msm_dp_cata */ u32 msm_dp_catalog_hw_revision(const struct msm_dp_catalog *msm_dp_catalog) { - const struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); - - return msm_dp_read_ahb(catalog, REG_DP_HW_VERSION); + return msm_dp_read_ahb(msm_dp_catalog, REG_DP_HW_VERSION); } /** @@ -547,28 +497,24 @@ u32 msm_dp_catalog_hw_revision(const struct msm_dp_catalog *msm_dp_catalog) void msm_dp_catalog_ctrl_reset(struct msm_dp_catalog *msm_dp_catalog) { u32 sw_reset; - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); - sw_reset = msm_dp_read_ahb(catalog, REG_DP_SW_RESET); + sw_reset = msm_dp_read_ahb(msm_dp_catalog, REG_DP_SW_RESET); sw_reset |= DP_SW_RESET; - msm_dp_write_ahb(catalog, REG_DP_SW_RESET, sw_reset); + msm_dp_write_ahb(msm_dp_catalog, REG_DP_SW_RESET, sw_reset); usleep_range(1000, 1100); /* h/w recommended delay */ sw_reset &= ~DP_SW_RESET; - msm_dp_write_ahb(catalog, REG_DP_SW_RESET, sw_reset); + msm_dp_write_ahb(msm_dp_catalog, REG_DP_SW_RESET, sw_reset); } bool msm_dp_catalog_ctrl_mainlink_ready(struct msm_dp_catalog *msm_dp_catalog) { u32 data; int ret; - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); /* Poll for mainlink ready status */ - ret = readl_poll_timeout(catalog->io.link.base + + ret = readl_poll_timeout(msm_dp_catalog->link_base + REG_DP_MAINLINK_READY, data, data & DP_MAINLINK_READY_FOR_VIDEO, POLLING_SLEEP_US, POLLING_TIMEOUT_US); @@ -583,17 +529,14 @@ bool msm_dp_catalog_ctrl_mainlink_ready(struct msm_dp_catalog *msm_dp_catalog) void msm_dp_catalog_ctrl_enable_irq(struct msm_dp_catalog *msm_dp_catalog, bool enable) { - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); - if (enable) { - msm_dp_write_ahb(catalog, REG_DP_INTR_STATUS, + msm_dp_write_ahb(msm_dp_catalog, REG_DP_INTR_STATUS, DP_INTERRUPT_STATUS1_MASK); - msm_dp_write_ahb(catalog, REG_DP_INTR_STATUS2, + msm_dp_write_ahb(msm_dp_catalog, REG_DP_INTR_STATUS2, DP_INTERRUPT_STATUS2_MASK); } else { - msm_dp_write_ahb(catalog, REG_DP_INTR_STATUS, 0x00); - msm_dp_write_ahb(catalog, REG_DP_INTR_STATUS2, 0x00); + msm_dp_write_ahb(msm_dp_catalog, REG_DP_INTR_STATUS, 0x00); + msm_dp_write_ahb(msm_dp_catalog, REG_DP_INTR_STATUS2, 0x00); } } @@ -603,73 +546,63 @@ void msm_dp_catalog_hpd_config_intr(struct msm_dp_catalog *msm_dp_catalog, struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, struct msm_dp_catalog_private, msm_dp_catalog); - u32 config = msm_dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK); + u32 config = msm_dp_read_aux(msm_dp_catalog, REG_DP_DP_HPD_INT_MASK); config = (en ? config | intr_mask : config & ~intr_mask); drm_dbg_dp(catalog->drm_dev, "intr_mask=%#x config=%#x\n", intr_mask, config); - msm_dp_write_aux(catalog, REG_DP_DP_HPD_INT_MASK, + msm_dp_write_aux(msm_dp_catalog, REG_DP_DP_HPD_INT_MASK, config & DP_DP_HPD_INT_MASK); } void msm_dp_catalog_ctrl_hpd_enable(struct msm_dp_catalog *msm_dp_catalog) { - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); - - u32 reftimer = msm_dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER); + u32 reftimer = msm_dp_read_aux(msm_dp_catalog, REG_DP_DP_HPD_REFTIMER); /* Configure REFTIMER and enable it */ reftimer |= DP_DP_HPD_REFTIMER_ENABLE; - msm_dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer); + msm_dp_write_aux(msm_dp_catalog, REG_DP_DP_HPD_REFTIMER, reftimer); /* Enable HPD */ - msm_dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, DP_DP_HPD_CTRL_HPD_EN); + msm_dp_write_aux(msm_dp_catalog, REG_DP_DP_HPD_CTRL, DP_DP_HPD_CTRL_HPD_EN); } void msm_dp_catalog_ctrl_hpd_disable(struct msm_dp_catalog *msm_dp_catalog) { - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); - - u32 reftimer = msm_dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER); + u32 reftimer = msm_dp_read_aux(msm_dp_catalog, REG_DP_DP_HPD_REFTIMER); reftimer &= ~DP_DP_HPD_REFTIMER_ENABLE; - msm_dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer); + msm_dp_write_aux(msm_dp_catalog, REG_DP_DP_HPD_REFTIMER, reftimer); - msm_dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, 0); + msm_dp_write_aux(msm_dp_catalog, REG_DP_DP_HPD_CTRL, 0); } -static void msm_dp_catalog_enable_sdp(struct msm_dp_catalog_private *catalog) +static void msm_dp_catalog_enable_sdp(struct msm_dp_catalog *msm_dp_catalog) { /* trigger sdp */ - msm_dp_write_link(catalog, MMSS_DP_SDP_CFG3, UPDATE_SDP); - msm_dp_write_link(catalog, MMSS_DP_SDP_CFG3, 0x0); + msm_dp_write_link(msm_dp_catalog, MMSS_DP_SDP_CFG3, UPDATE_SDP); + msm_dp_write_link(msm_dp_catalog, MMSS_DP_SDP_CFG3, 0x0); } void msm_dp_catalog_ctrl_config_psr(struct msm_dp_catalog *msm_dp_catalog) { - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); u32 config; /* enable PSR1 function */ - config = msm_dp_read_link(catalog, REG_PSR_CONFIG); + config = msm_dp_read_link(msm_dp_catalog, REG_PSR_CONFIG); config |= PSR1_SUPPORTED; - msm_dp_write_link(catalog, REG_PSR_CONFIG, config); + msm_dp_write_link(msm_dp_catalog, REG_PSR_CONFIG, config); - msm_dp_write_ahb(catalog, REG_DP_INTR_MASK4, DP_INTERRUPT_MASK4); - msm_dp_catalog_enable_sdp(catalog); + msm_dp_write_ahb(msm_dp_catalog, REG_DP_INTR_MASK4, DP_INTERRUPT_MASK4); + msm_dp_catalog_enable_sdp(msm_dp_catalog); } void msm_dp_catalog_ctrl_set_psr(struct msm_dp_catalog *msm_dp_catalog, bool enter) { - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); u32 cmd; - cmd = msm_dp_read_link(catalog, REG_PSR_CMD); + cmd = msm_dp_read_link(msm_dp_catalog, REG_PSR_CMD); cmd &= ~(PSR_ENTER | PSR_EXIT); @@ -678,8 +611,8 @@ void msm_dp_catalog_ctrl_set_psr(struct msm_dp_catalog *msm_dp_catalog, bool ent else cmd |= PSR_EXIT; - msm_dp_catalog_enable_sdp(catalog); - msm_dp_write_link(catalog, REG_PSR_CMD, cmd); + msm_dp_catalog_enable_sdp(msm_dp_catalog); + msm_dp_write_link(msm_dp_catalog, REG_PSR_CMD, cmd); } u32 msm_dp_catalog_link_is_connected(struct msm_dp_catalog *msm_dp_catalog) @@ -688,7 +621,7 @@ u32 msm_dp_catalog_link_is_connected(struct msm_dp_catalog *msm_dp_catalog) struct msm_dp_catalog_private, msm_dp_catalog); u32 status; - status = msm_dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS); + status = msm_dp_read_aux(msm_dp_catalog, REG_DP_DP_HPD_INT_STATUS); drm_dbg_dp(catalog->drm_dev, "aux status: %#x\n", status); status >>= DP_DP_HPD_STATE_STATUS_BITS_SHIFT; status &= DP_DP_HPD_STATE_STATUS_BITS_MASK; @@ -698,14 +631,12 @@ u32 msm_dp_catalog_link_is_connected(struct msm_dp_catalog *msm_dp_catalog) u32 msm_dp_catalog_hpd_get_intr_status(struct msm_dp_catalog *msm_dp_catalog) { - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); int isr, mask; - isr = msm_dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS); - msm_dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK, + isr = msm_dp_read_aux(msm_dp_catalog, REG_DP_DP_HPD_INT_STATUS); + msm_dp_write_aux(msm_dp_catalog, REG_DP_DP_HPD_INT_ACK, (isr & DP_DP_HPD_INT_MASK)); - mask = msm_dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK); + mask = msm_dp_read_aux(msm_dp_catalog, REG_DP_DP_HPD_INT_MASK); /* * We only want to return interrupts that are unmasked to the caller. @@ -719,29 +650,25 @@ u32 msm_dp_catalog_hpd_get_intr_status(struct msm_dp_catalog *msm_dp_catalog) u32 msm_dp_catalog_ctrl_read_psr_interrupt_status(struct msm_dp_catalog *msm_dp_catalog) { - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); u32 intr, intr_ack; - intr = msm_dp_read_ahb(catalog, REG_DP_INTR_STATUS4); + intr = msm_dp_read_ahb(msm_dp_catalog, REG_DP_INTR_STATUS4); intr_ack = (intr & DP_INTERRUPT_STATUS4) << DP_INTERRUPT_STATUS_ACK_SHIFT; - msm_dp_write_ahb(catalog, REG_DP_INTR_STATUS4, intr_ack); + msm_dp_write_ahb(msm_dp_catalog, REG_DP_INTR_STATUS4, intr_ack); return intr; } int msm_dp_catalog_ctrl_get_interrupt(struct msm_dp_catalog *msm_dp_catalog) { - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); u32 intr, intr_ack; - intr = msm_dp_read_ahb(catalog, REG_DP_INTR_STATUS2); + intr = msm_dp_read_ahb(msm_dp_catalog, REG_DP_INTR_STATUS2); intr &= ~DP_INTERRUPT_STATUS2_MASK; intr_ack = (intr & DP_INTERRUPT_STATUS2) << DP_INTERRUPT_STATUS_ACK_SHIFT; - msm_dp_write_ahb(catalog, REG_DP_INTR_STATUS2, + msm_dp_write_ahb(msm_dp_catalog, REG_DP_INTR_STATUS2, intr_ack | DP_INTERRUPT_STATUS2_MASK); return intr; @@ -749,13 +676,10 @@ int msm_dp_catalog_ctrl_get_interrupt(struct msm_dp_catalog *msm_dp_catalog) void msm_dp_catalog_ctrl_phy_reset(struct msm_dp_catalog *msm_dp_catalog) { - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); - - msm_dp_write_ahb(catalog, REG_DP_PHY_CTRL, + msm_dp_write_ahb(msm_dp_catalog, REG_DP_PHY_CTRL, DP_PHY_CTRL_SW_RESET | DP_PHY_CTRL_SW_RESET_PLL); usleep_range(1000, 1100); /* h/w recommended delay */ - msm_dp_write_ahb(catalog, REG_DP_PHY_CTRL, 0x0); + msm_dp_write_ahb(msm_dp_catalog, REG_DP_PHY_CTRL, 0x0); } void msm_dp_catalog_ctrl_send_phy_pattern(struct msm_dp_catalog *msm_dp_catalog, @@ -766,66 +690,66 @@ void msm_dp_catalog_ctrl_send_phy_pattern(struct msm_dp_catalog *msm_dp_catalog, u32 value = 0x0; /* Make sure to clear the current pattern before starting a new one */ - msm_dp_write_link(catalog, REG_DP_STATE_CTRL, 0x0); + msm_dp_write_link(msm_dp_catalog, REG_DP_STATE_CTRL, 0x0); drm_dbg_dp(catalog->drm_dev, "pattern: %#x\n", pattern); switch (pattern) { case DP_PHY_TEST_PATTERN_D10_2: - msm_dp_write_link(catalog, REG_DP_STATE_CTRL, + msm_dp_write_link(msm_dp_catalog, REG_DP_STATE_CTRL, DP_STATE_CTRL_LINK_TRAINING_PATTERN1); break; case DP_PHY_TEST_PATTERN_ERROR_COUNT: value &= ~(1 << 16); - msm_dp_write_link(catalog, REG_DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, + msm_dp_write_link(msm_dp_catalog, REG_DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, value); value |= SCRAMBLER_RESET_COUNT_VALUE; - msm_dp_write_link(catalog, REG_DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, + msm_dp_write_link(msm_dp_catalog, REG_DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, value); - msm_dp_write_link(catalog, REG_DP_MAINLINK_LEVELS, + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_LEVELS, DP_MAINLINK_SAFE_TO_EXIT_LEVEL_2); - msm_dp_write_link(catalog, REG_DP_STATE_CTRL, + msm_dp_write_link(msm_dp_catalog, REG_DP_STATE_CTRL, DP_STATE_CTRL_LINK_SYMBOL_ERR_MEASURE); break; case DP_PHY_TEST_PATTERN_PRBS7: - msm_dp_write_link(catalog, REG_DP_STATE_CTRL, + msm_dp_write_link(msm_dp_catalog, REG_DP_STATE_CTRL, DP_STATE_CTRL_LINK_PRBS7); break; case DP_PHY_TEST_PATTERN_80BIT_CUSTOM: - msm_dp_write_link(catalog, REG_DP_STATE_CTRL, + msm_dp_write_link(msm_dp_catalog, REG_DP_STATE_CTRL, DP_STATE_CTRL_LINK_TEST_CUSTOM_PATTERN); /* 00111110000011111000001111100000 */ - msm_dp_write_link(catalog, REG_DP_TEST_80BIT_CUSTOM_PATTERN_REG0, + msm_dp_write_link(msm_dp_catalog, REG_DP_TEST_80BIT_CUSTOM_PATTERN_REG0, 0x3E0F83E0); /* 00001111100000111110000011111000 */ - msm_dp_write_link(catalog, REG_DP_TEST_80BIT_CUSTOM_PATTERN_REG1, + msm_dp_write_link(msm_dp_catalog, REG_DP_TEST_80BIT_CUSTOM_PATTERN_REG1, 0x0F83E0F8); /* 1111100000111110 */ - msm_dp_write_link(catalog, REG_DP_TEST_80BIT_CUSTOM_PATTERN_REG2, + msm_dp_write_link(msm_dp_catalog, REG_DP_TEST_80BIT_CUSTOM_PATTERN_REG2, 0x0000F83E); break; case DP_PHY_TEST_PATTERN_CP2520: - value = msm_dp_read_link(catalog, REG_DP_MAINLINK_CTRL); + value = msm_dp_read_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL); value &= ~DP_MAINLINK_CTRL_SW_BYPASS_SCRAMBLER; - msm_dp_write_link(catalog, REG_DP_MAINLINK_CTRL, value); + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL, value); value = DP_HBR2_ERM_PATTERN; - msm_dp_write_link(catalog, REG_DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, + msm_dp_write_link(msm_dp_catalog, REG_DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, value); value |= SCRAMBLER_RESET_COUNT_VALUE; - msm_dp_write_link(catalog, REG_DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, + msm_dp_write_link(msm_dp_catalog, REG_DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, value); - msm_dp_write_link(catalog, REG_DP_MAINLINK_LEVELS, + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_LEVELS, DP_MAINLINK_SAFE_TO_EXIT_LEVEL_2); - msm_dp_write_link(catalog, REG_DP_STATE_CTRL, + msm_dp_write_link(msm_dp_catalog, REG_DP_STATE_CTRL, DP_STATE_CTRL_LINK_SYMBOL_ERR_MEASURE); - value = msm_dp_read_link(catalog, REG_DP_MAINLINK_CTRL); + value = msm_dp_read_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL); value |= DP_MAINLINK_CTRL_ENABLE; - msm_dp_write_link(catalog, REG_DP_MAINLINK_CTRL, value); + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL, value); break; case DP_PHY_TEST_PATTERN_SEL_MASK: - msm_dp_write_link(catalog, REG_DP_MAINLINK_CTRL, + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_CTRL, DP_MAINLINK_CTRL_ENABLE); - msm_dp_write_link(catalog, REG_DP_STATE_CTRL, + msm_dp_write_link(msm_dp_catalog, REG_DP_STATE_CTRL, DP_STATE_CTRL_LINK_TRAINING_PATTERN4); break; default: @@ -837,26 +761,21 @@ void msm_dp_catalog_ctrl_send_phy_pattern(struct msm_dp_catalog *msm_dp_catalog, u32 msm_dp_catalog_ctrl_read_phy_pattern(struct msm_dp_catalog *msm_dp_catalog) { - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); - - return msm_dp_read_link(catalog, REG_DP_MAINLINK_READY); + return msm_dp_read_link(msm_dp_catalog, REG_DP_MAINLINK_READY); } /* panel related catalog functions */ int msm_dp_catalog_panel_timing_cfg(struct msm_dp_catalog *msm_dp_catalog, u32 total, u32 sync_start, u32 width_blanking, u32 msm_dp_active) { - struct msm_dp_catalog_private *catalog = container_of(msm_dp_catalog, - struct msm_dp_catalog_private, msm_dp_catalog); u32 reg; - msm_dp_write_link(catalog, REG_DP_TOTAL_HOR_VER, total); - msm_dp_write_link(catalog, REG_DP_START_HOR_VER_FROM_SYNC, sync_start); - msm_dp_write_link(catalog, REG_DP_HSYNC_VSYNC_WIDTH_POLARITY, width_blanking); - msm_dp_write_link(catalog, REG_DP_ACTIVE_HOR_VER, msm_dp_active); + msm_dp_write_link(msm_dp_catalog, REG_DP_TOTAL_HOR_VER, total); + msm_dp_write_link(msm_dp_catalog, REG_DP_START_HOR_VER_FROM_SYNC, sync_start); + msm_dp_write_link(msm_dp_catalog, REG_DP_HSYNC_VSYNC_WIDTH_POLARITY, width_blanking); + msm_dp_write_link(msm_dp_catalog, REG_DP_ACTIVE_HOR_VER, msm_dp_active); - reg = msm_dp_read_p0(catalog, MMSS_DP_INTF_CONFIG); + reg = msm_dp_read_p0(msm_dp_catalog, MMSS_DP_INTF_CONFIG); if (msm_dp_catalog->wide_bus_en) reg |= DP_INTF_CONFIG_DATABUS_WIDEN; @@ -866,42 +785,36 @@ int msm_dp_catalog_panel_timing_cfg(struct msm_dp_catalog *msm_dp_catalog, u32 t DRM_DEBUG_DP("wide_bus_en=%d reg=%#x\n", msm_dp_catalog->wide_bus_en, reg); - msm_dp_write_p0(catalog, MMSS_DP_INTF_CONFIG, reg); + msm_dp_write_p0(msm_dp_catalog, MMSS_DP_INTF_CONFIG, reg); return 0; } static void msm_dp_catalog_panel_send_vsc_sdp(struct msm_dp_catalog *msm_dp_catalog, struct dp_sdp *vsc_sdp) { - struct msm_dp_catalog_private *catalog; u32 header[2]; u32 val; int i; - catalog = container_of(msm_dp_catalog, struct msm_dp_catalog_private, msm_dp_catalog); - msm_dp_utils_pack_sdp_header(&vsc_sdp->sdp_header, header); - msm_dp_write_link(catalog, MMSS_DP_GENERIC0_0, header[0]); - msm_dp_write_link(catalog, MMSS_DP_GENERIC0_1, header[1]); + msm_dp_write_link(msm_dp_catalog, MMSS_DP_GENERIC0_0, header[0]); + msm_dp_write_link(msm_dp_catalog, MMSS_DP_GENERIC0_1, header[1]); for (i = 0; i < sizeof(vsc_sdp->db); i += 4) { val = ((vsc_sdp->db[i]) | (vsc_sdp->db[i + 1] << 8) | (vsc_sdp->db[i + 2] << 16) | (vsc_sdp->db[i + 3] << 24)); - msm_dp_write_link(catalog, MMSS_DP_GENERIC0_2 + i, val); + msm_dp_write_link(msm_dp_catalog, MMSS_DP_GENERIC0_2 + i, val); } } static void msm_dp_catalog_panel_update_sdp(struct msm_dp_catalog *msm_dp_catalog) { - struct msm_dp_catalog_private *catalog; u32 hw_revision; - catalog = container_of(msm_dp_catalog, struct msm_dp_catalog_private, msm_dp_catalog); - hw_revision = msm_dp_catalog_hw_revision(msm_dp_catalog); if (hw_revision < DP_HW_VERSION_1_2 && hw_revision >= DP_HW_VERSION_1_0) { - msm_dp_write_link(catalog, MMSS_DP_SDP_CFG3, 0x01); - msm_dp_write_link(catalog, MMSS_DP_SDP_CFG3, 0x00); + msm_dp_write_link(msm_dp_catalog, MMSS_DP_SDP_CFG3, 0x01); + msm_dp_write_link(msm_dp_catalog, MMSS_DP_SDP_CFG3, 0x00); } } @@ -912,15 +825,15 @@ void msm_dp_catalog_panel_enable_vsc_sdp(struct msm_dp_catalog *msm_dp_catalog, catalog = container_of(msm_dp_catalog, struct msm_dp_catalog_private, msm_dp_catalog); - cfg = msm_dp_read_link(catalog, MMSS_DP_SDP_CFG); - cfg2 = msm_dp_read_link(catalog, MMSS_DP_SDP_CFG2); - misc = msm_dp_read_link(catalog, REG_DP_MISC1_MISC0); + cfg = msm_dp_read_link(msm_dp_catalog, MMSS_DP_SDP_CFG); + cfg2 = msm_dp_read_link(msm_dp_catalog, MMSS_DP_SDP_CFG2); + misc = msm_dp_read_link(msm_dp_catalog, REG_DP_MISC1_MISC0); cfg |= GEN0_SDP_EN; - msm_dp_write_link(catalog, MMSS_DP_SDP_CFG, cfg); + msm_dp_write_link(msm_dp_catalog, MMSS_DP_SDP_CFG, cfg); cfg2 |= GENERIC0_SDPSIZE_VALID; - msm_dp_write_link(catalog, MMSS_DP_SDP_CFG2, cfg2); + msm_dp_write_link(msm_dp_catalog, MMSS_DP_SDP_CFG2, cfg2); msm_dp_catalog_panel_send_vsc_sdp(msm_dp_catalog, vsc_sdp); @@ -930,7 +843,7 @@ void msm_dp_catalog_panel_enable_vsc_sdp(struct msm_dp_catalog *msm_dp_catalog, drm_dbg_dp(catalog->drm_dev, "vsc sdp enable=1\n"); pr_debug("misc settings = 0x%x\n", misc); - msm_dp_write_link(catalog, REG_DP_MISC1_MISC0, misc); + msm_dp_write_link(msm_dp_catalog, REG_DP_MISC1_MISC0, misc); msm_dp_catalog_panel_update_sdp(msm_dp_catalog); } @@ -942,15 +855,15 @@ void msm_dp_catalog_panel_disable_vsc_sdp(struct msm_dp_catalog *msm_dp_catalog) catalog = container_of(msm_dp_catalog, struct msm_dp_catalog_private, msm_dp_catalog); - cfg = msm_dp_read_link(catalog, MMSS_DP_SDP_CFG); - cfg2 = msm_dp_read_link(catalog, MMSS_DP_SDP_CFG2); - misc = msm_dp_read_link(catalog, REG_DP_MISC1_MISC0); + cfg = msm_dp_read_link(msm_dp_catalog, MMSS_DP_SDP_CFG); + cfg2 = msm_dp_read_link(msm_dp_catalog, MMSS_DP_SDP_CFG2); + misc = msm_dp_read_link(msm_dp_catalog, REG_DP_MISC1_MISC0); cfg &= ~GEN0_SDP_EN; - msm_dp_write_link(catalog, MMSS_DP_SDP_CFG, cfg); + msm_dp_write_link(msm_dp_catalog, MMSS_DP_SDP_CFG, cfg); cfg2 &= ~GENERIC0_SDPSIZE_VALID; - msm_dp_write_link(catalog, MMSS_DP_SDP_CFG2, cfg2); + msm_dp_write_link(msm_dp_catalog, MMSS_DP_SDP_CFG2, cfg2); /* switch back to MSA */ misc &= ~DP_MISC1_VSC_SDP; @@ -958,7 +871,7 @@ void msm_dp_catalog_panel_disable_vsc_sdp(struct msm_dp_catalog *msm_dp_catalog) drm_dbg_dp(catalog->drm_dev, "vsc sdp enable=0\n"); pr_debug("misc settings = 0x%x\n", misc); - msm_dp_write_link(catalog, REG_DP_MISC1_MISC0, misc); + msm_dp_write_link(msm_dp_catalog, REG_DP_MISC1_MISC0, misc); msm_dp_catalog_panel_update_sdp(msm_dp_catalog); } @@ -1055,15 +968,15 @@ static void __iomem *msm_dp_ioremap(struct platform_device *pdev, int idx, size_ static int msm_dp_catalog_get_io(struct msm_dp_catalog_private *catalog) { + struct msm_dp_catalog *msm_dp_catalog = &catalog->msm_dp_catalog; struct platform_device *pdev = to_platform_device(catalog->dev); - struct dss_io_data *dss = &catalog->io; - dss->ahb.base = msm_dp_ioremap(pdev, 0, &dss->ahb.len); - if (IS_ERR(dss->ahb.base)) - return PTR_ERR(dss->ahb.base); + msm_dp_catalog->ahb_base = msm_dp_ioremap(pdev, 0, &msm_dp_catalog->ahb_len); + if (IS_ERR(msm_dp_catalog->ahb_base)) + return PTR_ERR(msm_dp_catalog->ahb_base); - dss->aux.base = msm_dp_ioremap(pdev, 1, &dss->aux.len); - if (IS_ERR(dss->aux.base)) { + msm_dp_catalog->aux_base = msm_dp_ioremap(pdev, 1, &msm_dp_catalog->aux_len); + if (IS_ERR(msm_dp_catalog->aux_base)) { /* * The initial binding had a single reg, but in order to * support variation in the sub-region sizes this was split. @@ -1071,34 +984,34 @@ static int msm_dp_catalog_get_io(struct msm_dp_catalog_private *catalog) * reg is specified, so fill in the sub-region offsets and * lengths based on this single region. */ - if (PTR_ERR(dss->aux.base) == -EINVAL) { - if (dss->ahb.len < DP_DEFAULT_P0_OFFSET + DP_DEFAULT_P0_SIZE) { + if (PTR_ERR(msm_dp_catalog->aux_base) == -EINVAL) { + if (msm_dp_catalog->ahb_len < DP_DEFAULT_P0_OFFSET + DP_DEFAULT_P0_SIZE) { DRM_ERROR("legacy memory region not large enough\n"); return -EINVAL; } - dss->ahb.len = DP_DEFAULT_AHB_SIZE; - dss->aux.base = dss->ahb.base + DP_DEFAULT_AUX_OFFSET; - dss->aux.len = DP_DEFAULT_AUX_SIZE; - dss->link.base = dss->ahb.base + DP_DEFAULT_LINK_OFFSET; - dss->link.len = DP_DEFAULT_LINK_SIZE; - dss->p0.base = dss->ahb.base + DP_DEFAULT_P0_OFFSET; - dss->p0.len = DP_DEFAULT_P0_SIZE; + msm_dp_catalog->ahb_len = DP_DEFAULT_AHB_SIZE; + msm_dp_catalog->aux_base = msm_dp_catalog->ahb_base + DP_DEFAULT_AUX_OFFSET; + msm_dp_catalog->aux_len = DP_DEFAULT_AUX_SIZE; + msm_dp_catalog->link_base = msm_dp_catalog->ahb_base + DP_DEFAULT_LINK_OFFSET; + msm_dp_catalog->link_len = DP_DEFAULT_LINK_SIZE; + msm_dp_catalog->p0_base = msm_dp_catalog->ahb_base + DP_DEFAULT_P0_OFFSET; + msm_dp_catalog->p0_len = DP_DEFAULT_P0_SIZE; } else { - DRM_ERROR("unable to remap aux region: %pe\n", dss->aux.base); - return PTR_ERR(dss->aux.base); + DRM_ERROR("unable to remap aux region: %pe\n", msm_dp_catalog->aux_base); + return PTR_ERR(msm_dp_catalog->aux_base); } } else { - dss->link.base = msm_dp_ioremap(pdev, 2, &dss->link.len); - if (IS_ERR(dss->link.base)) { - DRM_ERROR("unable to remap link region: %pe\n", dss->link.base); - return PTR_ERR(dss->link.base); + msm_dp_catalog->link_base = msm_dp_ioremap(pdev, 2, &msm_dp_catalog->link_len); + if (IS_ERR(msm_dp_catalog->link_base)) { + DRM_ERROR("unable to remap link region: %pe\n", msm_dp_catalog->link_base); + return PTR_ERR(msm_dp_catalog->link_base); } - dss->p0.base = msm_dp_ioremap(pdev, 3, &dss->p0.len); - if (IS_ERR(dss->p0.base)) { - DRM_ERROR("unable to remap p0 region: %pe\n", dss->p0.base); - return PTR_ERR(dss->p0.base); + msm_dp_catalog->p0_base = msm_dp_ioremap(pdev, 3, &msm_dp_catalog->p0_len); + if (IS_ERR(msm_dp_catalog->p0_base)) { + DRM_ERROR("unable to remap p0 region: %pe\n", msm_dp_catalog->p0_base); + return PTR_ERR(msm_dp_catalog->p0_base); } } @@ -1135,7 +1048,7 @@ u32 msm_dp_catalog_audio_get_header(struct msm_dp_catalog *msm_dp_catalog, sdp_map = catalog->audio_map; - return msm_dp_read_link(catalog, sdp_map[sdp][header]); + return msm_dp_read_link(msm_dp_catalog, sdp_map[sdp][header]); } void msm_dp_catalog_audio_set_header(struct msm_dp_catalog *msm_dp_catalog, @@ -1154,7 +1067,7 @@ void msm_dp_catalog_audio_set_header(struct msm_dp_catalog *msm_dp_catalog, sdp_map = catalog->audio_map; - msm_dp_write_link(catalog, sdp_map[sdp][header], data); + msm_dp_write_link(msm_dp_catalog, sdp_map[sdp][header], data); } void msm_dp_catalog_audio_config_acr(struct msm_dp_catalog *msm_dp_catalog, u32 select) @@ -1173,7 +1086,7 @@ void msm_dp_catalog_audio_config_acr(struct msm_dp_catalog *msm_dp_catalog, u32 drm_dbg_dp(catalog->drm_dev, "select: %#x, acr_ctrl: %#x\n", select, acr_ctrl); - msm_dp_write_link(catalog, MMSS_DP_AUDIO_ACR_CTRL, acr_ctrl); + msm_dp_write_link(msm_dp_catalog, MMSS_DP_AUDIO_ACR_CTRL, acr_ctrl); } void msm_dp_catalog_audio_enable(struct msm_dp_catalog *msm_dp_catalog, bool enable) @@ -1187,7 +1100,7 @@ void msm_dp_catalog_audio_enable(struct msm_dp_catalog *msm_dp_catalog, bool ena catalog = container_of(msm_dp_catalog, struct msm_dp_catalog_private, msm_dp_catalog); - audio_ctrl = msm_dp_read_link(catalog, MMSS_DP_AUDIO_CFG); + audio_ctrl = msm_dp_read_link(msm_dp_catalog, MMSS_DP_AUDIO_CFG); if (enable) audio_ctrl |= BIT(0); @@ -1196,7 +1109,7 @@ void msm_dp_catalog_audio_enable(struct msm_dp_catalog *msm_dp_catalog, bool ena drm_dbg_dp(catalog->drm_dev, "dp_audio_cfg = 0x%x\n", audio_ctrl); - msm_dp_write_link(catalog, MMSS_DP_AUDIO_CFG, audio_ctrl); + msm_dp_write_link(msm_dp_catalog, MMSS_DP_AUDIO_CFG, audio_ctrl); /* make sure audio engine is disabled */ wmb(); } @@ -1213,7 +1126,7 @@ void msm_dp_catalog_audio_config_sdp(struct msm_dp_catalog *msm_dp_catalog) catalog = container_of(msm_dp_catalog, struct msm_dp_catalog_private, msm_dp_catalog); - sdp_cfg = msm_dp_read_link(catalog, MMSS_DP_SDP_CFG); + sdp_cfg = msm_dp_read_link(msm_dp_catalog, MMSS_DP_SDP_CFG); /* AUDIO_TIMESTAMP_SDP_EN */ sdp_cfg |= BIT(1); /* AUDIO_STREAM_SDP_EN */ @@ -1227,9 +1140,9 @@ void msm_dp_catalog_audio_config_sdp(struct msm_dp_catalog *msm_dp_catalog) drm_dbg_dp(catalog->drm_dev, "sdp_cfg = 0x%x\n", sdp_cfg); - msm_dp_write_link(catalog, MMSS_DP_SDP_CFG, sdp_cfg); + msm_dp_write_link(msm_dp_catalog, MMSS_DP_SDP_CFG, sdp_cfg); - sdp_cfg2 = msm_dp_read_link(catalog, MMSS_DP_SDP_CFG2); + sdp_cfg2 = msm_dp_read_link(msm_dp_catalog, MMSS_DP_SDP_CFG2); /* IFRM_REGSRC -> Do not use reg values */ sdp_cfg2 &= ~BIT(0); /* AUDIO_STREAM_HB3_REGSRC-> Do not use reg values */ @@ -1237,7 +1150,7 @@ void msm_dp_catalog_audio_config_sdp(struct msm_dp_catalog *msm_dp_catalog) drm_dbg_dp(catalog->drm_dev, "sdp_cfg2 = 0x%x\n", sdp_cfg2); - msm_dp_write_link(catalog, MMSS_DP_SDP_CFG2, sdp_cfg2); + msm_dp_write_link(msm_dp_catalog, MMSS_DP_SDP_CFG2, sdp_cfg2); } void msm_dp_catalog_audio_init(struct msm_dp_catalog *msm_dp_catalog) @@ -1292,7 +1205,7 @@ void msm_dp_catalog_audio_sfe_level(struct msm_dp_catalog *msm_dp_catalog, u32 s catalog = container_of(msm_dp_catalog, struct msm_dp_catalog_private, msm_dp_catalog); - mainlink_levels = msm_dp_read_link(catalog, REG_DP_MAINLINK_LEVELS); + mainlink_levels = msm_dp_read_link(msm_dp_catalog, REG_DP_MAINLINK_LEVELS); mainlink_levels &= 0xFE0; mainlink_levels |= safe_to_exit_level; @@ -1300,5 +1213,5 @@ void msm_dp_catalog_audio_sfe_level(struct msm_dp_catalog *msm_dp_catalog, u32 s "mainlink_level = 0x%x, safe_to_exit_level = 0x%x\n", mainlink_levels, safe_to_exit_level); - msm_dp_write_link(catalog, REG_DP_MAINLINK_LEVELS, mainlink_levels); + msm_dp_write_link(msm_dp_catalog, REG_DP_MAINLINK_LEVELS, mainlink_levels); } diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h index 62a401d8f75a6af06445a42af657d65e3fe542c5..13486c9c8703748e69e846be681951368df0a29e 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.h +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h @@ -49,6 +49,18 @@ enum msm_dp_catalog_audio_header_type { struct msm_dp_catalog { bool wide_bus_en; + + void __iomem *ahb_base; + size_t ahb_len; + + void __iomem *aux_base; + size_t aux_len; + + void __iomem *link_base; + size_t link_len; + + void __iomem *p0_base; + size_t p0_len; }; /* Debug module */