| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ca0a2eb3-21b6-d312-c8e0-61da48c4c700@gmail.com>
Date: Wed, 8 Feb 2017 15:23:53 -0800
From: Steve Longerbeam <slongerbeam@...il.com>
To: Philipp Zabel <p.zabel@...gutronix.de>
Cc: robh+dt@...nel.org, mark.rutland@....com, shawnguo@...nel.org,
kernel@...gutronix.de, fabio.estevam@....com,
linux@...linux.org.uk, mchehab@...nel.org, hverkuil@...all.nl,
nick@...anahar.org, markus.heiser@...marIT.de,
laurent.pinchart+renesas@...asonboard.com, bparrot@...com,
geert@...ux-m68k.org, arnd@...db.de, sudipm.mukherjee@...il.com,
minghsiu.tsai@...iatek.com, tiffany.lin@...iatek.com,
jean-christophe.trotin@...com, horms+renesas@...ge.net.au,
niklas.soderlund+renesas@...natech.se, robert.jarzmik@...e.fr,
songjun.wu@...rochip.com, andrew-ct.chen@...iatek.com,
gregkh@...uxfoundation.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-media@...r.kernel.org, devel@...verdev.osuosl.org,
Steve Longerbeam <steve_longerbeam@...tor.com>
Subject: Re: [PATCH v3 21/24] media: imx: Add MIPI CSI-2 Receiver subdev
driver
On 02/02/2017 03:50 AM, Philipp Zabel wrote:
>
>> + struct v4l2_subdev *src_sd;
>> + struct v4l2_subdev *sink_sd[CSI2_NUM_SRC_PADS];
> I see no reason to store pointers to the remote v4l2_subdevs.
>
>> + int input_pad;
>> + struct clk *dphy_clk;
>> + struct clk *cfg_clk;
>> + struct clk *pix_clk; /* what is this? */
>> + void __iomem *base;
>> + int intr1;
>> + int intr2;
> The interrupts are not used, I'd remove them and the dead code in
> _probe.
done.
>> +
>> +static inline u32 imxcsi2_read(struct imxcsi2_dev *csi2, unsigned int regoff)
>> +{
>> + return readl(csi2->base + regoff);
>> +}
>> +
>> +static inline void imxcsi2_write(struct imxcsi2_dev *csi2, u32 val,
>> + unsigned int regoff)
>> +{
>> + writel(val, csi2->base + regoff);
>> +}
> Do those two wrappers really make the code more readable?
It doesn't really matter to me either way, I removed these
macros.
>> +
>> +static void imxcsi2_enable(struct imxcsi2_dev *csi2, bool enable)
>> +{
>> + if (enable) {
>> + imxcsi2_write(csi2, 0xffffffff, CSI2_PHY_SHUTDOWNZ);
>> + imxcsi2_write(csi2, 0xffffffff, CSI2_DPHY_RSTZ);
>> + imxcsi2_write(csi2, 0xffffffff, CSI2_RESETN);
> Given that these registers only contain a single bit, and bits 31:1 are
> documented as reserved, 0, I think these should write 1 instead of
> 0xffffffff.
Yes, these lines are lifted from the FSL BSP's mipi csi-2 driver. I
did notice this but left it in place because I was worried about
possible undocumented bits. I tried writing 1 to these registers
and the behavior is the same as before (still works).
>> + } else {
>> + imxcsi2_write(csi2, 0x0, CSI2_PHY_SHUTDOWNZ);
>> + imxcsi2_write(csi2, 0x0, CSI2_DPHY_RSTZ);
>> + imxcsi2_write(csi2, 0x0, CSI2_RESETN);
>> + }
>> +}
>> +
>> +static void imxcsi2_reset(struct imxcsi2_dev *csi2)
>> +{
>> + imxcsi2_enable(csi2, false);
>> +
>> + imxcsi2_write(csi2, 0x00000001, CSI2_PHY_TST_CTRL0);
>> + imxcsi2_write(csi2, 0x00000000, CSI2_PHY_TST_CTRL1);
>> + imxcsi2_write(csi2, 0x00000000, CSI2_PHY_TST_CTRL0);
>> + imxcsi2_write(csi2, 0x00000002, CSI2_PHY_TST_CTRL0);
>> + imxcsi2_write(csi2, 0x00010044, CSI2_PHY_TST_CTRL1);
>> + imxcsi2_write(csi2, 0x00000000, CSI2_PHY_TST_CTRL0);
>> + imxcsi2_write(csi2, 0x00000014, CSI2_PHY_TST_CTRL1);
>> + imxcsi2_write(csi2, 0x00000002, CSI2_PHY_TST_CTRL0);
>> + imxcsi2_write(csi2, 0x00000000, CSI2_PHY_TST_CTRL0);
> These magic constants should be replaced with proper defines for the
> documented bitfields, if available.
>
> #define PHY_TESTCLR BIT(0)
> #define PHY_TESTCLK BIT(1)
>
> #define PHY_TESTEN BIT(16)
>
> /* Clear PHY test interface */
> imxcsi2_write(csi2, PHY_TESTCLR, CSI2_PHY_TST_CTRL0);
> imxcsi2_write(csi2, 0, CSI2_PHY_TST_CTRL1);
> imxcsi2_write(csi2, 0, CSI2_PHY_TST_CTRL0);
>
> /* Raise test interface strobe signal */
> imxcsi2_write(csi2, PHY_TESTCLK, CSI2_PHY_TST_CTRL0);
>
> /* Configure address write on falling edge and lower strobe signal */
> u8 addr = 0x44;
> imxcsi2_write(csi2, PHY_TESTEN | addr, CSI2_PHY_TST_CTRL1);
> imxcsi2_write(csi2, 0, CSI2_PHY_TST_CTRL0);
>
> /* Configure data write on rising edge and raise strobe signal */
> u8 data = 0x14;
> imxcsi2_write(csi2, data, CSI2_PHY_TST_CTRL1);
> imxcsi2_write(csi2, PHY_TESTCLK, CSI2_PHY_TST_CTRL0);
>
> /* Clear strobe signal */
> imxcsi2_write(csi2, 0, CSI2_PHY_TST_CTRL0);
>
> The whole sequence should probably be encapsulated in a
> dw_mipi_dphy_write function.
>
> Actually, this exact function already exists as dw_mipi_dsi_phy_write in
> drivers/gpu/drm/rockchip/dw-mipi-dsi.c, and it looks like the D-PHY
> register 0x44 might contain a field called HSFREQRANGE_SEL.
Thanks for pointing out drivers/gpu/drm/rockchip/dw-mipi-dsi.c.
It's clear from that driver that there probably needs to be a fuller
treatment of the D-PHY programming here, but I don't know where
to find the MIPI CSI-2 D-PHY documentation for the i.MX6. The code
in imxcsi2_reset() was also pulled from FSL, and that's all I really have
to go on for the D-PHY programming. I assume the D-PHY is also a
Synopsys core, like the host controller, but the i.MX6 manual doesn't
cover it.
In any case I've created dw_mipi_csi2_phy_write(), modeled after
dw_mipi_dsi_phy_write(). The "0x14" value is a value derived for
a target max bandwidth per lane of 300 Mbps, at least that is what
drivers/gpu/drm/rockchip/dw-mipi-dsi.c suggests. I've added a FIXME
note that effect, that this value should be derived based on the D-PHY
PLL clock rate and the desired max lane bandwidth.
>> + imxcsi2_enable(csi2, true);
>> +}
>> +
>> +static int imxcsi2_dphy_wait(struct imxcsi2_dev *csi2)
>> +{
>> + u32 reg;
>> + int i;
>> +
>> + /* wait for mipi sensor ready */
> More specifically, wait for the clock lane module to leave ULP state.
I've split this function in two:
imxcsi2_dphy_wait_lp_11()
which waits for !PHY_RXULPSCLKNOT, and stable/error-free
CSI-2 bus (CSI2_ERR1 == 0),
and
imxcsi2_dphy_wait_clock_lane()
which waits for PHY_RXCLKACTIVEHS.
The former is called during s_power(1), the latter in s_stream(1).
>> + for (i = 0; i < 50; i++) {
>> + reg = imxcsi2_read(csi2, CSI2_PHY_STATE);
>> + if (reg != 0x200)
> Magic constants are bad. This is PHY_RXULPSCLKNOT (clock lane module in
> ultra low power state).
Fixed.
>> + break;
>> + usleep_range(10000, 20000);
>> + }
> How about breaking this out into a wait function, or even better, using
> readl_poll_timeout instead of open coding these loops multiple times?
Cool, thanks for pointing out that macro. I've switched to
readl_poll_timeout()
everywhere.
>
>> +
>> + if (i >= 50) {
>> + v4l2_err(&csi2->sd,
>> + "wait for clock lane timeout, phy_state = 0x%08x\n",
>> + reg);
>> + return -ETIME;
>> + }
>> +
>> + /* wait for mipi stable */
> Wait for error free transmission?
Changed to /* wait until no errors on bus */.
>
>> +
>> + if (i >= 50) {
>> + v4l2_err(&csi2->sd,
>> + "wait for controller timeout, err1 = 0x%08x\n",
>> + reg);
>> + return -ETIME;
>> + }
>> +
>> + /* finally let's wait for active clock on the clock lane */
>> + for (i = 0; i < 50; i++) {
>> + reg = imxcsi2_read(csi2, CSI2_PHY_STATE);
>> + if (reg & (1 << 8))
> Yes, and that is PHY_RXCLKACTIVEHS.
done.
>> + return 0;
>> +}
>> +
>> +/*
>> + * V4L2 subdev operations
>> + */
>> +
>> +static int imxcsi2_link_setup(struct media_entity *entity,
>> + const struct media_pad *local,
>> + const struct media_pad *remote, u32 flags)
>> +{
>> + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
>> + struct imxcsi2_dev *csi2 = sd_to_dev(sd);
>> + struct v4l2_subdev *remote_sd;
>> +
>> + dev_dbg(csi2->dev, "link setup %s -> %s", remote->entity->name,
>> + local->entity->name);
>> +
>> + remote_sd = media_entity_to_v4l2_subdev(remote->entity);
>> +
>> + if (local->flags & MEDIA_PAD_FL_SOURCE) {
>> + if (flags & MEDIA_LNK_FL_ENABLED) {
>> + if (csi2->sink_sd[local->index])
>> + return -EBUSY;
>> + csi2->sink_sd[local->index] = remote_sd;
>> + } else {
>> + csi2->sink_sd[local->index] = NULL;
>> + }
>> + } else {
>> + if (flags & MEDIA_LNK_FL_ENABLED) {
>> + if (csi2->src_sd)
>> + return -EBUSY;
>> + csi2->src_sd = remote_sd;
>> + } else {
>> + csi2->src_sd = NULL;
>> + }
>> + }
> This whole code block is just to check if there is another link already
> active on the given pad. This could be stored in a boolean or a bit, no
> need to store pointers to remote subdevices.
I converted these to true booleans.
>
>> +
>> +static int imxcsi2_get_fmt(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_pad_config *cfg,
>> + struct v4l2_subdev_format *sdformat)
>> +{
>> + struct imxcsi2_dev *csi2 = sd_to_dev(sd);
>> +
>> + sdformat->format = csi2->format_mbus;
> The output formats are different from the input formats, see the media
> bus format discussion in the other thread. The input pad is the MIPI
> CSI-2 bus, but the four output pads are connected to the muxes / CSIs
> via a 16-bit parallel bus.
>
> So if the input format is UYVY8_1X16, for example, the output should be
> set to UYVY8_2X8.
Since the output buses from the CSI2IPU gasket are 16-bit
parallel buses, shouldn't an input format UYVY8_1X16 also be
the same at the output?
Steve
Powered by blists - more mailing lists