Messages in this thread Patch in this message | | | Date | Fri, 17 Sep 2021 22:58:11 +0530 | From | Pratyush Yadav <> | Subject | Re: [PATCH v5 2/6] phy: cdns-dphy: Add Rx support |
| |
+Rob
On 16/09/21 12:22PM, Paul Kocialkowski wrote: > Hi, > > On Fri 03 Sep 21, 00:25, Pratyush Yadav wrote: > > The Cadence DPHY can be used to receive image data over the CSI-2 > > protocol. Add support for Rx mode. The programming sequence differs from > > the Tx mode so it is added as a separate set of hooks to isolate the two > > paths. The mode in which the DPHY has to be used is selected based on > > the compatible. > > I just realized that I didn't follow-up on a previous revision on the debate > about using the phy sub-mode to distinguish between rx/tx. > > I see that you've been using a dedicated compatible, but I'm not sure this is a > good fit either. My understanding is that the compatible should describe a group > of register-compatible revisions of a hardware component, not how the hardware > is used specifically. I guess the distinction between rx/tx falls under > the latter rather than the former.
I am not sure if that is the case. For example, we use "ti,am654-ospi" for Cadence Quadspi controller. The default compatible, "cdns,qspi-nor", only supports Quad SPI (4 lines). The "ti,am654-ospi" compatible also supports Octal SPI (8 lines).
In addition, I feel like the Rx DPHY is almost a different type of device from a Tx DPHY. The programming sequence is completely different, the clocks required are different, etc. So I think using a different compatible for Rx mode makes sense.
Rob, what do you think?
> > I just sent out a patch in my Allwinner MIPI CSI-2+ISP series adding > a specific direction property: > - https://lists.infradead.org/pipermail/linux-phy/2021-September/001420.html > - https://lists.infradead.org/pipermail/linux-phy/2021-September/001421.html > > Which I feel is a more appropriate solution to implement the distinction. > > What do you think? > > Cheers, > > Paul > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > > > --- > > > > Changes in v5: > > - Use the new cdns_dphy_info to specify PHY ops. > > - Re-order include in alphabetical order. > > - Make bands const. > > - Drop num_bands. > > - Make i, lanes unsigned. > > - Drop the maximum check in cdns_dphy_rx_get_band_ctrl(). Let the loop > > complete and return -EOPNOTSUPP when we reach the end. > > - Drop the "rate < bands[i].min_rate" check since the bands are in > > ascending order. > > - Move data_lane_ctrl to start of function and make it static const. > > > > Changes in v4: > > - Drop the submode parts. Use a different compatible for the Rx ops. > > - Make bands and num_bands static. > > > > Changes in v3: > > - Use a table to select the band. > > - Use a table to poll the data lane ready bits. > > - Multiply the DPHY HS clock rate by 2 to get the bit rate since the > > clock is DDR. > > > > drivers/phy/cadence/cdns-dphy.c | 182 ++++++++++++++++++++++++++++++++ > > 1 file changed, 182 insertions(+) > > > > diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c > > index e2c2b27c6539..9a4c1c684e9a 100644 > > --- a/drivers/phy/cadence/cdns-dphy.c > > +++ b/drivers/phy/cadence/cdns-dphy.c > > @@ -1,11 +1,14 @@ > > // SPDX-License-Identifier: GPL-2.0+ > > /* > > * Copyright: 2017-2018 Cadence Design Systems, Inc. > > + * Copyright (C) 2021 Texas Instruments Incorporated - https://www.ti.com/ > > */ > > > > +#include <linux/bitfield.h> > > #include <linux/bitops.h> > > #include <linux/clk.h> > > #include <linux/io.h> > > +#include <linux/iopoll.h> > > #include <linux/module.h> > > #include <linux/of_address.h> > > #include <linux/of_device.h> > > @@ -25,10 +28,14 @@ > > #define DPHY_PMA_RCLK(reg) (0x600 + (reg)) > > #define DPHY_PMA_RDATA(lane, reg) (0x700 + ((lane) * 0x100) + (reg)) > > #define DPHY_PCS(reg) (0xb00 + (reg)) > > +#define DPHY_ISO(reg) (0xc00 + (reg)) > > > > #define DPHY_CMN_SSM DPHY_PMA_CMN(0x20) > > #define DPHY_CMN_SSM_EN BIT(0) > > +#define DPHY_CMN_RX_BANDGAP_TIMER_MASK GENMASK(8, 1) > > #define DPHY_CMN_TX_MODE_EN BIT(9) > > +#define DPHY_CMN_RX_MODE_EN BIT(10) > > +#define DPHY_CMN_RX_BANDGAP_TIMER 0x14 > > > > #define DPHY_CMN_PWM DPHY_PMA_CMN(0x40) > > #define DPHY_CMN_PWM_DIV(x) ((x) << 20) > > @@ -45,10 +52,27 @@ > > #define DPHY_CMN_OPDIV_FROM_REG BIT(6) > > #define DPHY_CMN_OPDIV(x) ((x) << 7) > > > > +#define DPHY_BAND_CFG DPHY_PCS(0x0) > > +#define DPHY_BAND_CFG_LEFT_BAND GENMASK(4, 0) > > +#define DPHY_BAND_CFG_RIGHT_BAND GENMASK(9, 5) > > + > > #define DPHY_PSM_CFG DPHY_PCS(0x4) > > #define DPHY_PSM_CFG_FROM_REG BIT(0) > > #define DPHY_PSM_CLK_DIV(x) ((x) << 1) > > > > +#define DPHY_POWER_ISLAND_EN_DATA DPHY_PCS(0x8) > > +#define DPHY_POWER_ISLAND_EN_DATA_VAL 0xaaaaaaaa > > +#define DPHY_POWER_ISLAND_EN_CLK DPHY_PCS(0xc) > > +#define DPHY_POWER_ISLAND_EN_CLK_VAL 0xaa > > + > > +#define DPHY_ISO_CL_CTRL_L DPHY_ISO(0x10) > > +#define DPHY_ISO_DL_CTRL_L0 DPHY_ISO(0x14) > > +#define DPHY_ISO_DL_CTRL_L1 DPHY_ISO(0x20) > > +#define DPHY_ISO_DL_CTRL_L2 DPHY_ISO(0x30) > > +#define DPHY_ISO_DL_CTRL_L3 DPHY_ISO(0x3c) > > +#define DPHY_ISO_LANE_READY_BIT 0 > > +#define DPHY_ISO_LANE_READY_TIMEOUT_MS 100UL > > + > > #define DSI_HBP_FRAME_OVERHEAD 12 > > #define DSI_HSA_FRAME_OVERHEAD 14 > > #define DSI_HFP_FRAME_OVERHEAD 6 > > @@ -57,6 +81,9 @@ > > #define DSI_NULL_FRAME_OVERHEAD 6 > > #define DSI_EOT_PKT_SIZE 4 > > > > +#define DPHY_LANES_MIN 1 > > +#define DPHY_LANES_MAX 4 > > + > > struct cdns_dphy_cfg { > > u8 pll_ipdiv; > > u8 pll_opdiv; > > @@ -105,6 +132,20 @@ struct cdns_dphy { > > struct phy *phy; > > }; > > > > +struct cdns_dphy_rx_band { > > + unsigned int min_rate; > > + unsigned int max_rate; > > +}; > > + > > +/* Order of bands is important since the index is the band number. */ > > +static const struct cdns_dphy_rx_band bands[] = { > > + {80, 100}, {100, 120}, {120, 160}, {160, 200}, {200, 240}, > > + {240, 280}, {280, 320}, {320, 360}, {360, 400}, {400, 480}, > > + {480, 560}, {560, 640}, {640, 720}, {720, 800}, {800, 880}, > > + {880, 1040}, {1040, 1200}, {1200, 1350}, {1350, 1500}, {1500, 1750}, > > + {1750, 2000}, {2000, 2250}, {2250, 2500} > > +}; > > + > > static int cdns_dsi_get_dphy_pll_cfg(struct cdns_dphy *dphy, > > struct cdns_dphy_cfg *cfg, > > struct phy_configure_opts_mipi_dphy *opts, > > @@ -360,6 +401,145 @@ static const struct cdns_dphy_info tx_ref_info = { > > .tx_ops = &tx_ref_dphy_ops, > > }; > > > > +static int cdns_dphy_rx_power_on(struct phy *phy) > > +{ > > + struct cdns_dphy *dphy = phy_get_drvdata(phy); > > + > > + /* Start RX state machine. */ > > + writel(DPHY_CMN_SSM_EN | DPHY_CMN_RX_MODE_EN | > > + FIELD_PREP(DPHY_CMN_RX_BANDGAP_TIMER_MASK, > > + DPHY_CMN_RX_BANDGAP_TIMER), > > + dphy->regs + DPHY_CMN_SSM); > > + > > + return 0; > > +} > > + > > +static int cdns_dphy_rx_power_off(struct phy *phy) > > +{ > > + struct cdns_dphy *dphy = phy_get_drvdata(phy); > > + > > + writel(0, dphy->regs + DPHY_CMN_SSM); > > + > > + return 0; > > +} > > + > > +static int cdns_dphy_rx_get_band_ctrl(unsigned long hs_clk_rate) > > +{ > > + unsigned int rate, i; > > + > > + rate = hs_clk_rate / 1000000UL; > > + /* Since CSI-2 clock is DDR, the bit rate is twice the clock rate. */ > > + rate *= 2; > > + > > + if (rate < bands[0].min_rate) > > + return -EOPNOTSUPP; > > + > > + for (i = 0; i < ARRAY_SIZE(bands); i++) { > > + if (rate < bands[i].max_rate) > > + return i; > > + } > > + > > + return -EOPNOTSUPP; > > +} > > + > > +static int cdns_dphy_rx_wait_for_bit(void __iomem *addr, unsigned int bit) > > +{ > > + u32 val; > > + > > + return readl_relaxed_poll_timeout(addr, val, val & BIT(bit), 10, > > + DPHY_ISO_LANE_READY_TIMEOUT_MS * 1000); > > +} > > + > > +static int cdns_dphy_rx_wait_lane_ready(struct cdns_dphy *dphy, > > + unsigned int lanes) > > +{ > > + static const u32 data_lane_ctrl[] = {DPHY_ISO_DL_CTRL_L0, > > + DPHY_ISO_DL_CTRL_L1, > > + DPHY_ISO_DL_CTRL_L2, > > + DPHY_ISO_DL_CTRL_L3}; > > + void __iomem *reg = dphy->regs; > > + unsigned int i; > > + int ret; > > + > > + /* Data lanes. Minimum one lane is mandatory. */ > > + if (lanes < DPHY_LANES_MIN || lanes > DPHY_LANES_MAX) > > + return -EINVAL; > > + > > + /* Clock lane */ > > + ret = cdns_dphy_rx_wait_for_bit(reg + DPHY_ISO_CL_CTRL_L, > > + DPHY_ISO_LANE_READY_BIT); > > + if (ret) > > + return ret; > > + > > + for (i = 0; i < lanes; i++) { > > + ret = cdns_dphy_rx_wait_for_bit(reg + data_lane_ctrl[i], > > + DPHY_ISO_LANE_READY_BIT); > > + if (ret) > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static int cdns_dphy_rx_configure(struct phy *phy, > > + union phy_configure_opts *opts) > > +{ > > + struct cdns_dphy *dphy = phy_get_drvdata(phy); > > + unsigned int reg; > > + int band_ctrl, ret; > > + > > + band_ctrl = cdns_dphy_rx_get_band_ctrl(opts->mipi_dphy.hs_clk_rate); > > + if (band_ctrl < 0) > > + return band_ctrl; > > + > > + reg = FIELD_PREP(DPHY_BAND_CFG_LEFT_BAND, band_ctrl) | > > + FIELD_PREP(DPHY_BAND_CFG_RIGHT_BAND, band_ctrl); > > + writel(reg, dphy->regs + DPHY_BAND_CFG); > > + > > + /* > > + * Set the required power island phase 2 time. This is mandated by DPHY > > + * specs. > > + */ > > + reg = DPHY_POWER_ISLAND_EN_DATA_VAL; > > + writel(reg, dphy->regs + DPHY_POWER_ISLAND_EN_DATA); > > + reg = DPHY_POWER_ISLAND_EN_CLK_VAL; > > + writel(reg, dphy->regs + DPHY_POWER_ISLAND_EN_CLK); > > + > > + ret = cdns_dphy_rx_wait_lane_ready(dphy, opts->mipi_dphy.lanes); > > + if (ret) { > > + dev_err(dphy->dev, "DPHY wait for lane ready timeout\n"); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static int cdns_dphy_rx_validate(struct phy *phy, enum phy_mode mode, > > + int submode, union phy_configure_opts *opts) > > +{ > > + int ret; > > + > > + if (mode != PHY_MODE_MIPI_DPHY) > > + return -EINVAL; > > + > > + ret = cdns_dphy_rx_get_band_ctrl(opts->mipi_dphy.hs_clk_rate); > > + if (ret < 0) > > + return ret; > > + > > + return phy_mipi_dphy_config_validate(&opts->mipi_dphy); > > +} > > + > > +static const struct phy_ops rx_ref_phy_ops = { > > + .power_on = cdns_dphy_rx_power_on, > > + .power_off = cdns_dphy_rx_power_off, > > + .configure = cdns_dphy_rx_configure, > > + .validate = cdns_dphy_rx_validate, > > +}; > > + > > +static const struct cdns_dphy_info rx_ref_info = { > > + .phy_ops = &rx_ref_phy_ops, > > +}; > > + > > static int cdns_dphy_probe(struct platform_device *pdev) > > { > > struct phy_provider *phy_provider; > > @@ -417,6 +597,7 @@ static int cdns_dphy_remove(struct platform_device *pdev) > > > > static const struct of_device_id cdns_dphy_of_match[] = { > > { .compatible = "cdns,dphy", .data = &tx_ref_info }, > > + { .compatible = "cdns,dphy-rx", .data = &rx_ref_info }, > > { /* sentinel */ }, > > }; > > MODULE_DEVICE_TABLE(of, cdns_dphy_of_match); > > @@ -432,5 +613,6 @@ static struct platform_driver cdns_dphy_platform_driver = { > > module_platform_driver(cdns_dphy_platform_driver); > > > > MODULE_AUTHOR("Maxime Ripard <maxime.ripard@bootlin.com>"); > > +MODULE_AUTHOR("Pratyush Yadav <p.yadav@ti.com>"); > > MODULE_DESCRIPTION("Cadence MIPI D-PHY Driver"); > > MODULE_LICENSE("GPL"); > > -- > > 2.33.0 > > > > -- > Paul Kocialkowski, Bootlin > Embedded Linux and kernel engineering > https://bootlin.com
-- Regards, Pratyush Yadav Texas Instruments Inc.
| |