Re: [PATCH 05/16] media: cadence: csi2rx: Add external DPHY support
On 31/03/21 05:24PM, Chunfeng Yun wrote:
> On Tue, 2021-03-30 at 23:03 +0530, Pratyush Yadav wrote:
> > Some platforms like TI's J721E can have the CSI2RX paired with an
> > external DPHY. Add support to enable and configure the DPHY using the
> > generic PHY framework.
> >
> > Get the pixel rate and bpp from the subdev and pass them on to the DPHY
> > along with the number of lanes. All other settings are left to their
> > default values.
> >
> > Signed-off-by: Pratyush Yadav
> > ---
> > drivers/media/platform/cadence/cdns-csi2rx.c | 147 +--
> > 1 file changed, 137 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c
> > b/drivers/media/platform/cadence/cdns-csi2rx.c
> > index c68a3eac62cd..31bd80e3f780 100644
> > --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> > @@ -30,6 +30,12 @@
> > #define CSI2RX_STATIC_CFG_DLANE_MAP(llane, plane) ((plane) << (16 +
> > (llane) * 4))
> > #define CSI2RX_STATIC_CFG_LANES_MASK GENMASK(11, 8)
> >
> > +#define CSI2RX_DPHY_LANE_CTRL_REG 0x40
> > +#define CSI2RX_DPHY_CL_RST BIT(16)
> > +#define CSI2RX_DPHY_DL_RST(i) BIT((i) + 12)
> > +#define CSI2RX_DPHY_CL_EN BIT(4)
> > +#define CSI2RX_DPHY_DL_EN(i) BIT(i)
> > +
> > #define CSI2RX_STREAM_BASE(n) (((n) + 1) * 0x100)
> >
> > #define CSI2RX_STREAM_CTRL_REG(n) (CSI2RX_STREAM_BASE(n) + 0x000)
> > @@ -54,6 +60,11 @@ enum csi2rx_pads {
> > CSI2RX_PAD_MAX,
> > };
> >
> > +struct csi2rx_fmt {
> > + u32 code;
> > + u8 bpp;
> > +};
> > +
> > struct csi2rx_priv {
> > struct device *dev;
> > unsigned intcount;
> > @@ -85,6 +96,52 @@ struct csi2rx_priv {
> > int source_pad;
> > };
> >
> > +static const struct csi2rx_fmt formats[] = {
> > + {
> > + .code = MEDIA_BUS_FMT_YUYV8_2X8,
> > + .bpp= 16,
> > + },
> > + {
> > + .code = MEDIA_BUS_FMT_UYVY8_2X8,
> > + .bpp= 16,
> > + },
> > + {
> > + .code = MEDIA_BUS_FMT_YVYU8_2X8,
> > + .bpp= 16,
> > + },
> > + {
> > + .code = MEDIA_BUS_FMT_VYUY8_2X8,
> > + .bpp= 16,
> > + },
> > +};
> > +
> > +static u8 csi2rx_get_bpp(u32 code)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(formats); i++) {
> > + if (formats[i].code == code)
> > + return formats[i].bpp;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static s64 csi2rx_get_pixel_rate(struct csi2rx_priv *csi2rx)
> > +{
> > + struct v4l2_ctrl *ctrl;
> > +
> > + ctrl = v4l2_ctrl_find(csi2rx->source_subdev->ctrl_handler,
> > + V4L2_CID_PIXEL_RATE);
> > + if (!ctrl) {
> > + dev_err(csi2rx->dev, "no pixel rate control in subdev: %s\n",
> > + csi2rx->source_subdev->name);
> > + return -EINVAL;
> > + }
> > +
> > + return v4l2_ctrl_g_ctrl_int64(ctrl);
> > +}
> > +
> > static inline
> > struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev)
> > {
> > @@ -101,6 +158,55 @@ static void csi2rx_reset(struct csi2rx_priv *csi2rx)
> > writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG);
> > }
> >
> > +static int csi2rx_configure_external_dphy(struct csi2rx_priv *csi2rx)
> > +{
> > + union phy_configure_opts opts = { };
> > + struct phy_configure_opts_mipi_dphy *cfg = _dphy;
> > + struct v4l2_subdev_format sd_fmt;
> > + s64 pixel_rate;
> > + int ret;
> > + u8 bpp;
> > +
> > + sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > + sd_fmt.pad = 0;
> > +
> > + ret = v4l2_subdev_call(csi2rx->source_subdev, pad, get_fmt, NULL,
> > + _fmt);
> > + if (ret)
> > + return ret;
> > +
> > + bpp = csi2rx_get_bpp(sd_fmt.format.code);
> > + if (!bpp)
> > + return -EINVAL;
> > +
> > + pixel_rate = csi2rx_get_pixel_rate(csi2rx);
> > + if (pixel_rate < 0)
> > + ret
Re: [PATCH 14/16] dt-bindings: phy: Convert Cadence DPHY binding to YAML
On 02/04/21 01:23PM, Laurent Pinchart wrote:
> Hi Pratyush,
>
> Thank you for the patch.
>
> On Tue, Mar 30, 2021 at 11:03:46PM +0530, Pratyush Yadav wrote:
> > Convert Cadence DPHY binding to YAML.
> >
> > Signed-off-by: Pratyush Yadav
> > ---
> > .../devicetree/bindings/phy/cdns,dphy.txt | 20
> > .../devicetree/bindings/phy/cdns,dphy.yaml| 51 +++
> > 2 files changed, 51 insertions(+), 20 deletions(-)
> > delete mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.txt
> > create mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.txt
> > b/Documentation/devicetree/bindings/phy/cdns,dphy.txt
> > deleted file mode 100644
> > index 1095bc4e72d9..
> > --- a/Documentation/devicetree/bindings/phy/cdns,dphy.txt
> > +++ /dev/null
> > @@ -1,20 +0,0 @@
> > -Cadence DPHY
> > -
> > -
> > -Cadence DPHY block.
> > -
> > -Required properties:
> > -- compatible: should be set to "cdns,dphy".
> > -- reg: physical base address and length of the DPHY registers.
> > -- clocks: DPHY reference clocks.
> > -- clock-names: must contain "psm" and "pll_ref".
> > -- #phy-cells: must be set to 0.
> > -
> > -Example:
> > - dphy0: dphy@fd0e{
> > - compatible = "cdns,dphy";
> > - reg = <0x0 0xfd0e 0x0 0x1000>;
> > - clocks = <_clk>, <_ref_clk>;
> > - clock-names = "psm", "pll_ref";
> > - #phy-cells = <0>;
> > - };
> > diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> > b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> > new file mode 100644
> > index ..d1bbf96a8250
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> > @@ -0,0 +1,51 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/phy/cdns,dphy.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Cadence DPHY Device Tree Bindings
> > +
> > +maintainers:
> > + - Pratyush Yadav
> > +
> > +properties:
> > + compatible:
> > +items:
> > + - const: cdns,dphy
> > +
> > + reg:
> > +maxItems: 1
> > +description: Physical base address and length of the DPHY registers.
>
> You can drop the description.
Ok.
>
> > +
> > + clocks:
> > +maxItems: 2
> > +description: DPHY reference clocks.
>
> It's best to describe individual items, which will then allow dropping
> the maxItems property:
>
> clocks:
> items:
> - description: Description of the psm clock
> - description: Description of the pll_ref clock
Ok. Though I'd mention that I am not 100% sure what the pll_ref clock is
supposed to be. The original binding doesn't have a description either.
>
> > +
> > + clock-names:
> > +items:
> > + - const: psm
> > + - const: pll_ref
> > +
> > + "#phy-cells":
> > +const: 0
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - clocks
> > + - clock-names
> > + - "#phy-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > +
> > +dphy0: dphy@fd0e{
>
> This is copied verbatim from the existing description, but while at it,
> I'd rename the node from dphy@... to phy@..., as DT node are supposed to
> be named according to the class of devices, not the specific device
> type.
Ok.
>
> With these small issues addressed,
>
> Reviewed-by: Laurent Pinchart
Thanks.
>
> > +compatible = "cdns,dphy";
> > +reg = <0xfd0e 0x1000>;
> > +clocks = <_clk>, <_ref_clk>;
> > +clock-names = "psm", "pll_ref";
> > +#phy-cells = <0>;
> > +};
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [PATCH 15/16] dt-bindings: phy: cdns,dphy: make clocks optional
On 02/04/21 01:31PM, Laurent Pinchart wrote: > Hi Pratyush, > > Thank you for the patch. > > On Tue, Mar 30, 2021 at 11:03:47PM +0530, Pratyush Yadav wrote: > > The clocks are not used by the DPHY when used in Rx mode so make them > > optional. > > Isn't there a main functional clock (DPHY_RX_MAIN_CLK in the J721E TRM) > that is needed in RX mode ? That clock is different from the clocks being used in this binding. The "psm" clock is for the PMA state machine (the internal state machine for the DPHY). The divider for this clock should be set such that the resultant clock is as close to 1 MHz as possible. This can be done either by programming the register value or by setting the correct value on the psm_clock_freq pin. On J721E the pin already has the correct value so there is no need for setting it via the register. The other clock is "pll_ref" which is used to set the input clock divider. Setting this divider is part of the DPHY TX programming sequence but is not part of the RX programming sequence. I'm not sure what exactly the divider does but I think it is supposed to divide the clock from the input stream to the TX DPHY to make sure the internal state machine is running at the correct speed. Anyway, it is not needed on the RX side because for that there is another register used (see cdns_dphy_rx_get_band_ctrl() in patch 4). The DPHY_RX_MAIN_CLK does eventually get divided into the PSM clock but it is not used directly. > > > Signed-off-by: Pratyush Yadav > > --- > > Documentation/devicetree/bindings/phy/cdns,dphy.yaml | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml > > b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml > > index d1bbf96a8250..0807ba68284d 100644 > > --- a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml > > +++ b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml > > @@ -33,8 +33,6 @@ properties: > > required: > >- compatible > >- reg > > - - clocks > > - - clock-names > >- "#phy-cells" > > > > additionalProperties: false > > -- > Regards, > > Laurent Pinchart -- Regards, Pratyush Yadav Texas Instruments Inc.
Re: [PATCH 16/16] dt-bindings: phy: cdns,dphy: add power-domains property
On 02/04/21 01:35PM, Laurent Pinchart wrote: > Hi Pratyush, > > Thank you for the patch. > > On Tue, Mar 30, 2021 at 11:03:48PM +0530, Pratyush Yadav wrote: > > This property is needed on TI platforms to enable the PD of the DPHY > > before it can be used. > > > > Signed-off-by: Pratyush Yadav > > --- > > Documentation/devicetree/bindings/phy/cdns,dphy.yaml | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml > > b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml > > index 0807ba68284d..ddcd4de0aef6 100644 > > --- a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml > > +++ b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml > > @@ -30,6 +30,9 @@ properties: > >"#phy-cells": > > const: 0 > > > > + power-domains: > > +maxItems: 1 > > + > > Would it be useful to add power-domains to the example ? Ok. Will add it in v2. > > Reviewed-by: Laurent Pinchart Thanks. > > > required: > >- compatible > >- reg > > -- > Regards, > > Laurent Pinchart -- Regards, Pratyush Yadav Texas Instruments Inc.
Re: [PATCH 03/16] phy: cdns-dphy: Allow setting mode
On 02/04/21 01:38PM, Laurent Pinchart wrote:
> Hi Pratyush,
>
> Thank you for the patch.
>
> On Tue, Mar 30, 2021 at 11:03:35PM +0530, Pratyush Yadav wrote:
> > Allow callers to set the PHY mode. The main mode should always be
> > PHY_MODE_MIPI_DPHY but the submode can either be
> > PHY_MIPI_DPHY_SUBMODE_RX or PHY_MIPI_DPHY_SUBMODE_TX. Update the ops
> > based on the requested submode.
>
> Isn't a given DPHY instance always meant to work in one particular mode
> ? I can't really imagine a single instance of this IP core being
> integrated in a way that it can be used in either RX or TX mode. It
> seems better to select the mode through DT, by describing if the DPHY is
> an RX or TX (possibly through different compatible strings).
I'm not sure if the DPHY can work in both RX and TX mode but the
documentation for Cadence DPHY on J721E does include both RX and TX
related registers. Also, take a look at [0] which says that the
Allwinner A31 DPHY can work in both RX and TX mode. So apparently there
are some DPHYs like that in the wild.
[0]
https://lore.kernel.org/linux-arm-kernel/20201023174546.504028-3-paul.kocialkow...@bootlin.com/
>
> > Signed-off-by: Pratyush Yadav
> > ---
> > drivers/phy/cadence/cdns-dphy.c | 30 ++
> > 1 file changed, 30 insertions(+)
> >
> > diff --git a/drivers/phy/cadence/cdns-dphy.c
> > b/drivers/phy/cadence/cdns-dphy.c
> > index 8656f2102a91..7d5f7b333893 100644
> > --- a/drivers/phy/cadence/cdns-dphy.c
> > +++ b/drivers/phy/cadence/cdns-dphy.c
> > @@ -365,11 +365,41 @@ static int cdns_dphy_configure(struct phy *phy, union
> > phy_configure_opts *opts)
> > return 0;
> > }
> >
> > +static int cdns_dphy_set_mode(struct phy *phy, enum phy_mode mode, int
> > submode)
> > +{
> > + struct cdns_dphy *dphy = phy_get_drvdata(phy);
> > + const struct cdns_dphy_driver_data *ddata;
> > +
> > + ddata = of_device_get_match_data(dphy->dev);
> > + if (!ddata)
> > + return -EINVAL;
> > +
> > + if (mode != PHY_MODE_MIPI_DPHY)
> > + return -EINVAL;
> > +
> > + if (submode == PHY_MIPI_DPHY_SUBMODE_TX) {
> > + if (!ddata->tx)
> > + return -EOPNOTSUPP;
> > +
> > + dphy->ops = ddata->tx;
> > + } else if (submode == PHY_MIPI_DPHY_SUBMODE_RX) {
> > + if (!ddata->rx)
> > + return -EOPNOTSUPP;
> > +
> > + dphy->ops = ddata->rx;
> > + } else {
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static const struct phy_ops cdns_dphy_ops = {
> > .configure = cdns_dphy_configure,
> > .validate = cdns_dphy_validate,
> > .power_on = cdns_dphy_power_on,
> > .power_off = cdns_dphy_power_off,
> > + .set_mode = cdns_dphy_set_mode,
> > };
> >
> > static int cdns_dphy_probe(struct platform_device *pdev)
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [PATCH 12/16] dt-bindings: media: Add DT bindings for TI CSI2RX driver
On 02/04/21 01:53PM, Laurent Pinchart wrote:
> On Fri, Apr 02, 2021 at 01:01:22PM +0300, Laurent Pinchart wrote:
> > On Thu, Apr 01, 2021 at 10:52:01AM -0500, Rob Herring wrote:
> > > On Tue, Mar 30, 2021 at 11:03:44PM +0530, Pratyush Yadav wrote:
> > > > TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate
> > > > capture over a CSI-2 bus. The TI CSI2RX platform driver glues all the
> > > > parts together.
> > > >
> > > > Signed-off-by: Pratyush Yadav
> > > > ---
> > > > .../devicetree/bindings/media/ti,csi2rx.yaml | 70 +++
> > > > 1 file changed, 70 insertions(+)
> > > > create mode 100644
> > > > Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > > > b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > > > new file mode 100644
> > > > index ..ebd894364391
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > > > @@ -0,0 +1,70 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/media/ti,csi2rx.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: TI CSI2RX Wrapper Device Tree Bindings
> > > > +
> >
> > A description would be useful, especially given that the TRM doesn't
> > mention "CSI2RX".
> >
> > > > +maintainers:
> > > > + - Pratyush Yadav
> > > > +
> > > > +properties:
> > > > + compatible:
> > > > +items:
> > > > + - const: ti,csi2rx
> > > > +
> > > > + dmas:
> > > > +description: RX DMA Channel 0
> > >
> > > items:
> > > - description: RX DMA Channel 0
> > >
> > > Or just 'maxItems: 1'
> > >
> > > > +
> > > > + dma-names:
> > > > +items:
> > > > + - const: rx0
> > > > +
> > > > + reg:
> > > > +maxItems: 1
> > > > +description: Base address and size of the TI wrapper registers.
> > >
> > > That's all 'reg' properties, drop 'description'.
> >
> > According to SPRUIL1B, there are four register banks for the CSI_RX_IF,
> > and two register banks for the DPHY_RX. What's your plan to support
> > these ? Not everything need to be implemented at once, but backward
> > compatibility need to be taken into account in the design.
> >
> > > > +
> > > > + power-domains:
> > > > +maxItems: 1
> > > > +description:
> > > > + PM domain provider node and an args specifier containing
> > > > + the device id value.
> > >
> > > Drop.
> > >
> > > > +
> > > > + ranges: true
> > > > +
> > > > + "#address-cells":
> > > > +const: 2
> > > > +
> > > > + "#size-cells":
> > > > +const: 2
> > > > +
> > > > +patternProperties:
> > > > + "csi-bridge@":
> > >
> > > "^csi-bridge@"
> > >
> > > > +type: object
> > > > +description: CSI2 bridge node.
> > >
> > > Just an empty node?
> >
> > Even if the node is optional, it would be useful to include it in the
> > example below, to show how it's supposed to be used.
> >
> > > > +
> > > > +required:
> > > > + - compatible
> > > > + - reg
> > > > + - dmas
> > > > + - dma-names
> > > > + - power-domains
> > > > + - "#address-cells"
> > > > + - "#size-cells"
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > + - |
> > > > +#include
> > > > +
> > > > +ti_csi2rx0: ticsi2rx {
> > > > +compatible = "ti,csi2rx";
> > > > +dmas = <_udmap 0x4940>;
> > > > +dma-names = "rx0";
> > > > +reg = <0x0 0x450 0x0 0x1000>;
> > > > +power-domains = <_pds 26 TI_SCI_PD_EXCLUSIVE>;
> > > > +#address-cells = <2>;
> > > > +#size-cells = <2>;
> > > > +};
>
> It would also be useful to expand this to a full example that includes
> integration with the PHY.
Integration with PHY is Cadence CSI2RX schema's problem. But I will add
the subnode here anyway so it should have the PHY related properties as
well.
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [PATCH 12/16] dt-bindings: media: Add DT bindings for TI CSI2RX driver
On 02/04/21 01:01PM, Laurent Pinchart wrote:
> On Thu, Apr 01, 2021 at 10:52:01AM -0500, Rob Herring wrote:
> > On Tue, Mar 30, 2021 at 11:03:44PM +0530, Pratyush Yadav wrote:
> > > TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate
> > > capture over a CSI-2 bus. The TI CSI2RX platform driver glues all the
> > > parts together.
> > >
> > > Signed-off-by: Pratyush Yadav
> > > ---
> > > .../devicetree/bindings/media/ti,csi2rx.yaml | 70 +++
> > > 1 file changed, 70 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > > b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > > new file mode 100644
> > > index ..ebd894364391
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > > @@ -0,0 +1,70 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/ti,csi2rx.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: TI CSI2RX Wrapper Device Tree Bindings
> > > +
>
> A description would be useful, especially given that the TRM doesn't
> mention "CSI2RX".
Ok.
>
> > > +maintainers:
> > > + - Pratyush Yadav
> > > +
> > > +properties:
> > > + compatible:
> > > +items:
> > > + - const: ti,csi2rx
> > > +
> > > + dmas:
> > > +description: RX DMA Channel 0
> >
> > items:
> > - description: RX DMA Channel 0
> >
> > Or just 'maxItems: 1'
> >
> > > +
> > > + dma-names:
> > > +items:
> > > + - const: rx0
> > > +
> > > + reg:
> > > +maxItems: 1
> > > +description: Base address and size of the TI wrapper registers.
> >
> > That's all 'reg' properties, drop 'description'.
>
> According to SPRUIL1B, there are four register banks for the CSI_RX_IF,
> and two register banks for the DPHY_RX. What's your plan to support
> these ? Not everything need to be implemented at once, but backward
> compatibility need to be taken into account in the design.
The CSI_RX_IF0_ECC_AGGR_CFG register bank is for safety requirements.
The driver does not use them. The CSI_RX_IF0_RX_SHIM_VBUSP_MMR_CSI2RXIF
register bank is for the TI wrapper around the Cadence CSI2RX bridge
that deals with DMA threads. This bank is what this binding is concerned
with. The CSI_RX_IF0_VBUS2APB_WRAP_VBUS_APB_CSI2RX bank is for the
Cadence CSI2RX bridge. The Cadence schema should deal with that. And
lastly, I don't know what the CSI_RX_IF0_CP_INTD_CFG_INTD_CFG bank is
for. The driver does not use it.
I don't forsee the first and last bank being used in Kernel, but if we
want to be safe I can change maxItems to 3. Sounds good?
>
> > > +
> > > + power-domains:
> > > +maxItems: 1
> > > +description:
> > > + PM domain provider node and an args specifier containing
> > > + the device id value.
> >
> > Drop.
> >
> > > +
> > > + ranges: true
> > > +
> > > + "#address-cells":
> > > +const: 2
> > > +
> > > + "#size-cells":
> > > +const: 2
> > > +
> > > +patternProperties:
> > > + "csi-bridge@":
> >
> > "^csi-bridge@"
> >
> > > +type: object
> > > +description: CSI2 bridge node.
> >
> > Just an empty node?
>
> Even if the node is optional, it would be useful to include it in the
> example below, to show how it's supposed to be used.
It is not optional. It should be the Cadence CSI2RX bridge node. Will
add it in the example. I also need to see if there is any way to make a
patternProperty a required property.
>
> > > +
> > > +required:
> > > + - compatible
> > > + - reg
> > > + - dmas
> > > + - dma-names
> > > + - power-domains
> > > + - "#address-cells"
> > > + - "#size-cells"
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > + - |
> > > +#include
> > > +
> > > +ti_csi2rx0: ticsi2rx {
> > > +compatible = "ti,csi2rx";
> > > +dmas = <_udmap 0x4940>;
> > > +dma-names = "rx0";
> > > +reg = <0x0 0x450 0x0 0x1000>;
> > > +power-domains = <_pds 26 TI_SCI_PD_EXCLUSIVE>;
> > > +#address-cells = <2>;
> > > +#size-cells = <2>;
> > > +};
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [PATCH 12/16] dt-bindings: media: Add DT bindings for TI CSI2RX driver
On 01/04/21 10:52AM, Rob Herring wrote:
> On Tue, Mar 30, 2021 at 11:03:44PM +0530, Pratyush Yadav wrote:
> > TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate
> > capture over a CSI-2 bus. The TI CSI2RX platform driver glues all the
> > parts together.
> >
> > Signed-off-by: Pratyush Yadav
> > ---
> > .../devicetree/bindings/media/ti,csi2rx.yaml | 70 +++
> > 1 file changed, 70 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > new file mode 100644
> > index ..ebd894364391
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > @@ -0,0 +1,70 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/ti,csi2rx.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: TI CSI2RX Wrapper Device Tree Bindings
> > +
> > +maintainers:
> > + - Pratyush Yadav
> > +
> > +properties:
> > + compatible:
> > +items:
> > + - const: ti,csi2rx
> > +
> > + dmas:
> > +description: RX DMA Channel 0
>
> items:
> - description: RX DMA Channel 0
>
> Or just 'maxItems: 1'
Ok.
>
> > +
> > + dma-names:
> > +items:
> > + - const: rx0
> > +
> > + reg:
> > +maxItems: 1
> > +description: Base address and size of the TI wrapper registers.
>
> That's all 'reg' properties, drop 'description'.
Ok.
>
> > +
> > + power-domains:
> > +maxItems: 1
> > +description:
> > + PM domain provider node and an args specifier containing
> > + the device id value.
>
> Drop.
Ok.
>
> > +
> > + ranges: true
> > +
> > + "#address-cells":
> > +const: 2
> > +
> > + "#size-cells":
> > +const: 2
> > +
> > +patternProperties:
> > + "csi-bridge@":
>
> "^csi-bridge@"
Ok.
>
> > +type: object
> > +description: CSI2 bridge node.
>
> Just an empty node?
No. It should be a node for the Cadence csi2rx IP (compatible
"cdns,csi2rx"). I'm not sure how to model this. This subnode is needed
but it should take its properties from the Cadence csi2rx schema. Will a
properties:
allOf:
- $ref: cdns,csi2rx.yaml#
be a good idea?
>
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - dmas
> > + - dma-names
> > + - power-domains
> > + - "#address-cells"
> > + - "#size-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > +#include
> > +
> > +ti_csi2rx0: ticsi2rx {
> > +compatible = "ti,csi2rx";
> > +dmas = <_udmap 0x4940>;
> > +dma-names = "rx0";
> > +reg = <0x0 0x450 0x0 0x1000>;
> > +power-domains = <_pds 26 TI_SCI_PD_EXCLUSIVE>;
> > +#address-cells = <2>;
> > +#size-cells = <2>;
> > +};
> > --
> > 2.30.0
> >
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [PATCH 09/16] media: cadence: csi2rx: Turn subdev power on before starting stream
On 02/04/21 01:55PM, Laurent Pinchart wrote:
> Hi Pratyush,
>
> Thank you for the patch.
>
> On Tue, Mar 30, 2021 at 11:03:41PM +0530, Pratyush Yadav wrote:
> > The subdevice power needs to be turned on before the stream is started.
> > Otherwise it might not be in the proper state to stream the data. Turn
> > it off when stopping the stream.
>
> The .s_power() operation is deprecated. Subdev drivers should control
> power internally in their .s_stream() operation, and they should use
> runtime PM to do so (this will allow usage of runtime PM autosuspend, to
> avoid expensive power off/on cycles when stopping and restarting video
> capture).
The s_power documentation in v4l2-subdev.h does not mention that it is
depreciated. A documentation update is in order. I will send a separate
patch to do it.
I tested this with OV5640. Not doing an s_power() operation before
s_stream() does not work. The application freezes forever waiting for
the first frame. So the OV5640 driver needs to be updated to use runtime
PM to do this, right?
>
> > Signed-off-by: Pratyush Yadav
> > ---
> > drivers/media/platform/cadence/cdns-csi2rx.c | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c
> > b/drivers/media/platform/cadence/cdns-csi2rx.c
> > index 7d1ac51e0698..3385e1bc213e 100644
> > --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> > @@ -256,6 +256,10 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
> >
> > writel(reg, csi2rx->base + CSI2RX_STATIC_CFG_REG);
> >
> > + ret = v4l2_subdev_call(csi2rx->source_subdev, core, s_power, true);
> > + if (ret && ret != -ENOIOCTLCMD)
> > + goto err_disable_pclk;
> > +
> > ret = v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, true);
> > if (ret)
> > goto err_disable_pclk;
> > @@ -358,6 +362,10 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
> > if (v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, false))
> > dev_warn(csi2rx->dev, "Couldn't disable our subdev\n");
> >
> > + ret = v4l2_subdev_call(csi2rx->source_subdev, core, s_power, false);
> > + if (ret && ret != -ENOIOCTLCMD)
> > + dev_warn(csi2rx->dev, "Couldn't power off subdev\n");
> > +
> > if (csi2rx->dphy) {
> > writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG);
> >
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [PATCH 11/16] dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX
On 06/04/21 10:25PM, Pratyush Yadav wrote: > On 06/04/21 06:33PM, Péter Ujfalusi wrote: > > > > > > On 4/6/21 6:09 PM, Pratyush Yadav wrote: > > > On 04/04/21 04:24PM, Péter Ujfalusi wrote: > > >> Hi Pratyush, > > >> > > >> On 3/30/21 8:33 PM, Pratyush Yadav wrote: > > >>> The CSI2RX subsystem uses PSI-L DMA to transfer frames to memory. It can > > >>> have up to 32 threads but the current driver only supports using one. So > > >>> add an entry for that one thread. > > >> > > >> If you are absolutely sure that the other threads are not going to be > > >> used, then: > > > > > > The opposite in fact. I do expect other threads to be used in the > > > future. But the current driver can only use one so I figured it is > > > better to add just the thread that is currently needed and then I can > > > always add the rest later. > > > > > > Why does this have to be a one-and-done deal? Is there anything wrong > > > with adding the other threads when the driver can actually use them? > > > > You can skip CCing DMAengine (and me ;) ). Less subsystems is the better > > when sending patches... > > I'm a bit confused here. If you are no longer interested in maintaining > the TI DMA drivers then that's fine, I can skip CCing you. But the patch > is still relevant to the dmaengine list so why should I skip CCing it? > And if I don't CC the dmaengine list then on which list would I get > comments/reviews for the patch? Ignore this. Got your point. Will do it in v2. -- Regards, Pratyush Yadav Texas Instruments Inc.
Re: [PATCH 11/16] dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX
On 06/04/21 06:33PM, Péter Ujfalusi wrote:
>
>
> On 4/6/21 6:09 PM, Pratyush Yadav wrote:
> > On 04/04/21 04:24PM, Péter Ujfalusi wrote:
> >> Hi Pratyush,
> >>
> >> On 3/30/21 8:33 PM, Pratyush Yadav wrote:
> >>> The CSI2RX subsystem uses PSI-L DMA to transfer frames to memory. It can
> >>> have up to 32 threads but the current driver only supports using one. So
> >>> add an entry for that one thread.
> >>
> >> If you are absolutely sure that the other threads are not going to be
> >> used, then:
> >
> > The opposite in fact. I do expect other threads to be used in the
> > future. But the current driver can only use one so I figured it is
> > better to add just the thread that is currently needed and then I can
> > always add the rest later.
> >
> > Why does this have to be a one-and-done deal? Is there anything wrong
> > with adding the other threads when the driver can actually use them?
>
> You can skip CCing DMAengine (and me ;) ). Less subsystems is the better
> when sending patches...
I'm a bit confused here. If you are no longer interested in maintaining
the TI DMA drivers then that's fine, I can skip CCing you. But the patch
is still relevant to the dmaengine list so why should I skip CCing it?
And if I don't CC the dmaengine list then on which list would I get
comments/reviews for the patch?
>
> >
> >> Acked-by: Peter Ujfalusi
> >>
> >> but I would consider adding the other threads if there is a chance that
> >> the cs2rx will need to support it in the future.
> >>
> >>> Signed-off-by: Pratyush Yadav
> >>> ---
> >>> drivers/dma/ti/k3-psil-j721e.c | 10 ++
> >>> 1 file changed, 10 insertions(+)
> >>>
> >>> diff --git a/drivers/dma/ti/k3-psil-j721e.c
> >>> b/drivers/dma/ti/k3-psil-j721e.c
> >>> index 7580870ed746..19ffa31e6dc6 100644
> >>> --- a/drivers/dma/ti/k3-psil-j721e.c
> >>> +++ b/drivers/dma/ti/k3-psil-j721e.c
> >>> @@ -58,6 +58,14 @@
> >>> }, \
> >>> }
> >>>
> >>> +#define PSIL_CSI2RX(x) \
> >>> + { \
> >>> + .thread_id = x, \
> >>> + .ep_config = { \
> >>> + .ep_type = PSIL_EP_NATIVE, \
> >>> + }, \
> >>> + }
> >>> +
> >>> /* PSI-L source thread IDs, used for RX (DMA_DEV_TO_MEM) */
> >>> static struct psil_ep j721e_src_ep_map[] = {
> >>> /* SA2UL */
> >>> @@ -138,6 +146,8 @@ static struct psil_ep j721e_src_ep_map[] = {
> >>> PSIL_PDMA_XY_PKT(0x4707),
> >>> PSIL_PDMA_XY_PKT(0x4708),
> >>> PSIL_PDMA_XY_PKT(0x4709),
> >>> + /* CSI2RX */
> >>> + PSIL_CSI2RX(0x4940),
> >>> /* CPSW9 */
> >>> PSIL_ETHERNET(0x4a00),
> >>> /* CPSW0 */
> >>>
> >>
> >> --
> >> Péter
> >
>
> --
> Péter
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [PATCH 10/16] media: cadence: csi2rx: Add wrappers for subdev calls
On 02/04/21 01:47PM, Laurent Pinchart wrote:
> Hi Pratyush,
>
> Thank you for the patch.
Thank you for the review :-)
>
> On Tue, Mar 30, 2021 at 11:03:42PM +0530, Pratyush Yadav wrote:
> > When this bridge driver is being user by another platform driver, it
> > might want to call subdev operations like getting format, setting
> > format, enumerating format codes, etc. Add wrapper functions that pass
> > that call through to the sensor.
> >
> > Currently wrappers are added only for the ops used by TI's platform
> > driver. More can be added later as needed.
>
> This isn't the direction we want to take. For new platforms, propagation
> of subdev configuration should be handled by userspace, using the V4L2
> userspace subdev API. This subdev should not call any subdev operation
> from its source other than .s_stream().
Right. I have replied to Tomi's message about this too. Will move the
driver to use the media controller API.
>
> > Signed-off-by: Pratyush Yadav
> > ---
> > drivers/media/platform/cadence/cdns-csi2rx.c | 77
> > 1 file changed, 77 insertions(+)
> >
> > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c
> > b/drivers/media/platform/cadence/cdns-csi2rx.c
> > index 3385e1bc213e..2e8bbc53cb8b 100644
> > --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> > @@ -408,12 +408,89 @@ static int csi2rx_s_stream(struct v4l2_subdev
> > *subdev, int enable)
> > return ret;
> > }
> >
> > +static int csi2rx_g_frame_interval(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_frame_interval *fi)
> > +{
> > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> > +
> > + return v4l2_subdev_call(csi2rx->source_subdev, video, g_frame_interval,
> > + fi);
> > +}
> > +
> > +static int csi2rx_s_frame_interval(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_frame_interval *fi)
> > +{
> > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> > +
> > + return v4l2_subdev_call(csi2rx->source_subdev, video, s_frame_interval,
> > + fi);
> > +}
> > +
> > +static int csi2rx_enum_mbus_code(struct v4l2_subdev *subdev,
> > +struct v4l2_subdev_pad_config *cfg,
> > +struct v4l2_subdev_mbus_code_enum *code)
> > +{
> > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> > +
> > + return v4l2_subdev_call(csi2rx->source_subdev, pad, enum_mbus_code,
> > + cfg, code);
> > +}
> > +
> > +static int csi2rx_get_fmt(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_format *fmt)
> > +{
> > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> > +
> > + return v4l2_subdev_call(csi2rx->source_subdev, pad, get_fmt, cfg, fmt);
> > +}
> > +
> > +static int csi2rx_set_fmt(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_format *fmt)
> > +{
> > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> > +
> > + return v4l2_subdev_call(csi2rx->source_subdev, pad, set_fmt, cfg, fmt);
> > +}
> > +
> > +static int csi2rx_enum_frame_size(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_frame_size_enum *fse)
> > +{
> > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> > +
> > + return v4l2_subdev_call(csi2rx->source_subdev, pad, enum_frame_size,
> > + cfg, fse);
> > +}
> > +
> > +static int csi2rx_enum_frame_interval(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_frame_interval_enum
> > *fie)
> > +{
> > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> > +
> > + return v4l2_subdev_call(csi2rx->source_subdev, pad, enum_frame_interval,
> > + cfg, fie);
> > +}
> > +
> > static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
> > .s_stream = csi2rx_s_stream,
> > + .g_frame_interval = csi2rx_g_frame_interval,
>
Re: [PATCH 11/16] dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX
On 04/04/21 04:24PM, Péter Ujfalusi wrote:
> Hi Pratyush,
>
> On 3/30/21 8:33 PM, Pratyush Yadav wrote:
> > The CSI2RX subsystem uses PSI-L DMA to transfer frames to memory. It can
> > have up to 32 threads but the current driver only supports using one. So
> > add an entry for that one thread.
>
> If you are absolutely sure that the other threads are not going to be
> used, then:
The opposite in fact. I do expect other threads to be used in the
future. But the current driver can only use one so I figured it is
better to add just the thread that is currently needed and then I can
always add the rest later.
Why does this have to be a one-and-done deal? Is there anything wrong
with adding the other threads when the driver can actually use them?
> Acked-by: Peter Ujfalusi
>
> but I would consider adding the other threads if there is a chance that
> the cs2rx will need to support it in the future.
>
> > Signed-off-by: Pratyush Yadav
> > ---
> > drivers/dma/ti/k3-psil-j721e.c | 10 ++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/dma/ti/k3-psil-j721e.c b/drivers/dma/ti/k3-psil-j721e.c
> > index 7580870ed746..19ffa31e6dc6 100644
> > --- a/drivers/dma/ti/k3-psil-j721e.c
> > +++ b/drivers/dma/ti/k3-psil-j721e.c
> > @@ -58,6 +58,14 @@
> > }, \
> > }
> >
> > +#define PSIL_CSI2RX(x) \
> > + { \
> > + .thread_id = x, \
> > + .ep_config = { \
> > + .ep_type = PSIL_EP_NATIVE, \
> > + }, \
> > + }
> > +
> > /* PSI-L source thread IDs, used for RX (DMA_DEV_TO_MEM) */
> > static struct psil_ep j721e_src_ep_map[] = {
> > /* SA2UL */
> > @@ -138,6 +146,8 @@ static struct psil_ep j721e_src_ep_map[] = {
> > PSIL_PDMA_XY_PKT(0x4707),
> > PSIL_PDMA_XY_PKT(0x4708),
> > PSIL_PDMA_XY_PKT(0x4709),
> > + /* CSI2RX */
> > + PSIL_CSI2RX(0x4940),
> > /* CPSW9 */
> > PSIL_ETHERNET(0x4a00),
> > /* CPSW0 */
> >
>
> --
> Péter
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [PATCH 13/16] media: ti-vpe: csi2rx: Add CSI2RX support
On 31/03/21 09:06AM, Tomi Valkeinen wrote: > Hi, > > On 30/03/2021 20:33, Pratyush Yadav wrote: > > TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate > > capture over a CSI-2 bus. > > > > The Cadence CSI2RX IP acts as a bridge between the TI specific parts and > > the CSI-2 protocol parts. TI then has a wrapper on top of this bridge > > called the SHIM layer. It takes in data from stream 0, repacks it, and > > sends it to memory over PSI-L DMA. > > > > This driver acts as the "front end" to V4L2 client applications. It > > implements the required ioctls and buffer operations, passes the > > necessary calls on to the bridge, programs the SHIM layer, and performs > > DMA via the dmaengine API to finally return the data to a buffer > > supplied by the application. > > > > Signed-off-by: Pratyush Yadav > > --- > > MAINTAINERS | 7 + > > drivers/media/platform/Kconfig| 11 + > > drivers/media/platform/ti-vpe/Makefile| 1 + > > drivers/media/platform/ti-vpe/ti-csi2rx.c | 964 ++ > > 4 files changed, 983 insertions(+) > > create mode 100644 drivers/media/platform/ti-vpe/ti-csi2rx.c > > Some quick comments: > > "ti-vpe" directory is not correct, this has nothing to do with VPE. That > said, the directory has already been abused by having CAL driver there, > perhaps we should rename the directory just to "ti". But if we do that, I > think we should have subdirs for cal, vpe and this new one. Right. I thought about doing this but then figured "let's just follow what CAL does". Will move them into separate subdirectories. > > "ti-csi2rx" is rather generic name. TI has had CSI-2 RX IPs before (CAL) and > probably will also have new ones in the future. If there's no clear model > name for the IP, as I think is the case here, it's probably best to just use > the SoC model in the name. E.g. the DSS on J7 is "ti,j721e-dss". Ok. > > This driver implements the legacy video API. I think it would be better (and > easier to maintain) to only implement the media-controller API, unless you > specifically need to support the legacy API for existing userspace. :-( I'm afraid the documentation has let me down here. There is no clear mention about the fact that media controller API replaces the "legacy" API. In fact, [0] seems to suggest that the media controller API is optional. Bridge drivers traditionally expose one or multiple video nodes to userspace, and control subdevices through the v4l2_subdev_ops operations in response to video node operations. This hides the complexity of the underlying hardware from applications. For complex devices, finer-grained control of the device than what the video nodes offer may be required. In those cases, bridge drivers that implement the media controller API may opt for making the subdevice operations directly accessible from userpace. Anyway, I don't think supporting the legacy API makes much sense since this is a new driver. Will convert it to use the MC API. We can always add the legacy API support if some application demands it. [0] https://www.kernel.org/doc/html/latest/driver-api/media/v4l2-subdev.html#v4l2-sub-device-userspace-api -- Regards, Pratyush Yadav Texas Instruments Inc.
Re: [PATCH 4/4] dt-bindings: spi: Convert cadence-quadspi.txt to cadence-quadspi.yaml
On 01/04/21 03:13PM, Mark Brown wrote: > On Thu, Apr 01, 2021 at 01:09:32AM +0530, Pratyush Yadav wrote: > > > I did take a look by running git log on > > Documentation/devicetree/bindings/spi/ and there is no single style > > being used. Using "dt-bindings: spi:" is a popular choice. Some other > > commits just use "spi:". And then some use "spi: dt-bindings:". The last > > commit to touch cadence-quadspi.txt (fcebca39938f) used the prefix > > "dt-bindings: spi:". > > Yes, lots of people pick unfortunate subject lines for DT patches - that > doesn't mean it's good. I'm looking to see spi: same as for all other > SPI patches. All right. "spi: dt-bindings:" it is from now on. > > > So on the prefix front I think the subject is good enough. Of course, if > > you have any other preference then it can be re-worded but let's first > > be clear on what the expectation is. And then let's make sure to apply > > it to all future patches uniformly. This way future contributors won't > > have to take a guess on what the expected prefix is. > > I do edit some percentage of patches, but some do slip through for > various reasons. There's also some things that just get completely > missed, especially if there isn't also a code patch nearby. > > > Apart from the prefix is there anything else to improve? IMHO the > > subject is good enough but I'm open to suggestions. > > There was the thing with constraints. Will send a follow up patch to add the constraints that Vignesh suggested. -- Regards, Pratyush Yadav Texas Instruments Inc.
Re: [PATCH 1/2] Revert "mtd: spi-nor: macronix: Add support for mx25l51245g"
On 02/04/21 11:20AM, Tudor Ambarus wrote:
> This reverts commit 04b8edad262eec0d153005973dfbdd83423c0dcb.
>
> mx25l51245g and mx66l51235l have the same flash ID. The flash
> detection returns the first entry in the flash_info array that
> matches the flash ID that was read, thus for the 0xc2201a ID,
> mx25l51245g was always hit, introducing a regression for
> mx66l51235l.
>
> If one wants to differentiate the flash names, a better fix would be
> to differentiate between the two at run-time, depending on SFDP,
> and choose the correct name from a list of flash names, depending on
> the SFDP differentiator.
>
> Fixes: 04b8edad262e ("mtd: spi-nor: macronix: Add support for mx25l51245g")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Tudor Ambarus
Acked-by: Pratyush Yadav
> ---
> drivers/mtd/spi-nor/macronix.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> index 6c2680b4cdad..42c2cf31702e 100644
> --- a/drivers/mtd/spi-nor/macronix.c
> +++ b/drivers/mtd/spi-nor/macronix.c
> @@ -72,9 +72,6 @@ static const struct flash_info macronix_parts[] = {
> SECT_4K | SPI_NOR_DUAL_READ |
> SPI_NOR_QUAD_READ) },
> { "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) },
> - { "mx25l51245g", INFO(0xc2201a, 0, 64 * 1024, 1024,
> - SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> - SPI_NOR_4B_OPCODES) },
> { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024,
> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> SPI_NOR_4B_OPCODES) },
> --
> 2.25.1
>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [PATCH 4/4] dt-bindings: spi: Convert cadence-quadspi.txt to cadence-quadspi.yaml
On 01/04/21 01:57PM, Vignesh Raghavendra wrote: > > > On 3/29/21 11:52 PM, Pratyush Yadav wrote: > >>> + cdns,fifo-depth: > >>> +description: > >>> + Size of the data FIFO in words. > >>> +$ref: "/schemas/types.yaml#/definitions/uint32" > >>> +enum: [ 128, 256 ] > >>> +default: 128 > >>> + > >>> + cdns,fifo-width: > >>> +$ref: /schemas/types.yaml#/definitions/uint32 > >>> +description: > >>> + Bus width of the data FIFO in bytes. > >>> +default: 4 > >> I assume there's some constraints on this? > > IIUC this is a matter of how the peripheral is implemented and there are > > no clear constraints. Different implementations can use different bus > > widths for the SRAM bus but I don't see any mention of minimum or > > maximum values. FWIW, all users in the kernel use a 4 byte bus. > > > > IMO a safe constraint would be to set a range of 1 to 4 (8/16/24/32 bit > wide) given this represents SRAM bus width. Binding can always be > updated if there exists an implementation with higher SRAM bus > width/fifo-width (although that's highly unlikely given there are no > such examples today). > > But leaving it open ended with range of 0 to UINT_MAX sounds incorrect > to me. Ok. Will respin. > > >> With that, > >> > >> Reviewed-by: Rob Herring > > Thanks. > > -- Regards, Pratyush Yadav Texas Instruments Inc.
Re: [PATCH 4/4] dt-bindings: spi: Convert cadence-quadspi.txt to cadence-quadspi.yaml
On 31/03/21 08:11PM, Mark Brown wrote: > On Fri, Mar 26, 2021 at 06:30:34PM +0530, Pratyush Yadav wrote: > > From: Ramuthevar Vadivel Murugan > > > > > > There is no way as of now to have a parent or bus defining properties > > for child nodes. For now, avoid it in the example to silence warnings on > > dt_schema_check. We can figure out how to deal with actual dts files > > later. > > Please submit patches using subject lines reflecting the style for the > subsystem, this makes it easier for people to identify relevant patches. > Look at what existing commits in the area you're changing are doing and > make sure your subject lines visually resemble what they're doing. > There's no need to resubmit to fix this alone. I did take a look by running git log on Documentation/devicetree/bindings/spi/ and there is no single style being used. Using "dt-bindings: spi:" is a popular choice. Some other commits just use "spi:". And then some use "spi: dt-bindings:". The last commit to touch cadence-quadspi.txt (fcebca39938f) used the prefix "dt-bindings: spi:". So on the prefix front I think the subject is good enough. Of course, if you have any other preference then it can be re-worded but let's first be clear on what the expectation is. And then let's make sure to apply it to all future patches uniformly. This way future contributors won't have to take a guess on what the expected prefix is. Apart from the prefix is there anything else to improve? IMHO the subject is good enough but I'm open to suggestions. -- Regards, Pratyush Yadav Texas Instruments Inc.
Re: [PATCH 00/16] CSI2RX support on J721E
On 31/03/21 06:36PM, Vinod Koul wrote: > On 31-03-21, 17:10, Pratyush Yadav wrote: > > On 31/03/21 03:03PM, Vinod Koul wrote: > > > On 30-03-21, 23:03, Pratyush Yadav wrote: > > > > Hi, > > > > > > > > This series adds support for CSI2 capture on J721E. It includes some > > > > fixes to the Cadence CSI2RX driver, adds Rx support to Cadence DPHY > > > > driver, and finally adds the TI CSI2RX wrapper driver. > > > > > > > > Tested on TI's J721E with OV5640 sensor. > > > > > > > > Paul Kocialkowski (1): > > > > phy: Distinguish between Rx and Tx for MIPI D-PHY with submodes > > > > > > > > Pratyush Yadav (15): > > > > phy: cdns-dphy: Prepare for Rx support > > > > phy: cdns-dphy: Allow setting mode > > > > phy: cdns-dphy: Add Rx support > > > > media: cadence: csi2rx: Add external DPHY support > > > > media: cadence: csi2rx: Soft reset the streams before starting capture > > > > media: cadence: csi2rx: Set the STOP bit when stopping a stream > > > > media: cadence: csi2rx: Fix stream data configuration > > > > media: cadence: csi2rx: Turn subdev power on before starting stream > > > > media: cadence: csi2rx: Add wrappers for subdev calls > > > > dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX > > > > dt-bindings: media: Add DT bindings for TI CSI2RX driver > > > > media: ti-vpe: csi2rx: Add CSI2RX support > > > > dt-bindings: phy: Convert Cadence DPHY binding to YAML > > > > dt-bindings: phy: cdns,dphy: make clocks optional > > > > dt-bindings: phy: cdns,dphy: add power-domains property > > > > > > Is there any dependency between patches to various subsystems, if not > > > please do consider sending a series per subsystem... > > > > Without patch 1, patch 5 and later won't build. Without patch 11, patch > > 13 will not work. > > Cover letter is a good place to mention this! And what do you mean by > not working, do we have compile time dependencies? I agree that for > everything to work all the pieces need to land I have not tried it but patch 11 is not a compile time dependency. It should be a run time dependency. Without it the driver will probably fail to acquire the DMA channels and its probe will fail. -- Regards, Pratyush Yadav Texas Instruments Inc.
Re: [PATCH 00/16] CSI2RX support on J721E
On 31/03/21 03:03PM, Vinod Koul wrote: > On 30-03-21, 23:03, Pratyush Yadav wrote: > > Hi, > > > > This series adds support for CSI2 capture on J721E. It includes some > > fixes to the Cadence CSI2RX driver, adds Rx support to Cadence DPHY > > driver, and finally adds the TI CSI2RX wrapper driver. > > > > Tested on TI's J721E with OV5640 sensor. > > > > Paul Kocialkowski (1): > > phy: Distinguish between Rx and Tx for MIPI D-PHY with submodes > > > > Pratyush Yadav (15): > > phy: cdns-dphy: Prepare for Rx support > > phy: cdns-dphy: Allow setting mode > > phy: cdns-dphy: Add Rx support > > media: cadence: csi2rx: Add external DPHY support > > media: cadence: csi2rx: Soft reset the streams before starting capture > > media: cadence: csi2rx: Set the STOP bit when stopping a stream > > media: cadence: csi2rx: Fix stream data configuration > > media: cadence: csi2rx: Turn subdev power on before starting stream > > media: cadence: csi2rx: Add wrappers for subdev calls > > dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX > > dt-bindings: media: Add DT bindings for TI CSI2RX driver > > media: ti-vpe: csi2rx: Add CSI2RX support > > dt-bindings: phy: Convert Cadence DPHY binding to YAML > > dt-bindings: phy: cdns,dphy: make clocks optional > > dt-bindings: phy: cdns,dphy: add power-domains property > > Is there any dependency between patches to various subsystems, if not > please do consider sending a series per subsystem... Without patch 1, patch 5 and later won't build. Without patch 11, patch 13 will not work. > > Thanks > > > > > > .../devicetree/bindings/media/ti,csi2rx.yaml | 70 ++ > > .../devicetree/bindings/phy/cdns,dphy.txt | 20 - > > .../devicetree/bindings/phy/cdns,dphy.yaml| 52 + > > MAINTAINERS | 7 + > > drivers/dma/ti/k3-psil-j721e.c| 10 + > > drivers/media/platform/Kconfig| 11 + > > drivers/media/platform/cadence/cdns-csi2rx.c | 269 - > > drivers/media/platform/ti-vpe/Makefile| 1 + > > drivers/media/platform/ti-vpe/ti-csi2rx.c | 964 ++ > > drivers/phy/cadence/cdns-dphy.c | 407 +++- > > include/linux/phy/phy-mipi-dphy.h | 13 + > > 11 files changed, 1754 insertions(+), 70 deletions(-) > > create mode 100644 Documentation/devicetree/bindings/media/ti,csi2rx.yaml > > delete mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.txt > > create mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.yaml > > create mode 100644 drivers/media/platform/ti-vpe/ti-csi2rx.c > > > > -- > > 2.30.0 > > -- > ~Vinod -- Regards, Pratyush Yadav Texas Instruments Inc.
[PATCH 16/16] dt-bindings: phy: cdns,dphy: add power-domains property
This property is needed on TI platforms to enable the PD of the DPHY before it can be used. Signed-off-by: Pratyush Yadav --- Documentation/devicetree/bindings/phy/cdns,dphy.yaml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml index 0807ba68284d..ddcd4de0aef6 100644 --- a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml +++ b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml @@ -30,6 +30,9 @@ properties: "#phy-cells": const: 0 + power-domains: +maxItems: 1 + required: - compatible - reg -- 2.30.0
[PATCH 15/16] dt-bindings: phy: cdns,dphy: make clocks optional
The clocks are not used by the DPHY when used in Rx mode so make them optional. Signed-off-by: Pratyush Yadav --- Documentation/devicetree/bindings/phy/cdns,dphy.yaml | 2 -- 1 file changed, 2 deletions(-) diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml index d1bbf96a8250..0807ba68284d 100644 --- a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml +++ b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml @@ -33,8 +33,6 @@ properties: required: - compatible - reg - - clocks - - clock-names - "#phy-cells" additionalProperties: false -- 2.30.0
[PATCH 14/16] dt-bindings: phy: Convert Cadence DPHY binding to YAML
Convert Cadence DPHY binding to YAML.
Signed-off-by: Pratyush Yadav
---
.../devicetree/bindings/phy/cdns,dphy.txt | 20
.../devicetree/bindings/phy/cdns,dphy.yaml| 51 +++
2 files changed, 51 insertions(+), 20 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.txt
create mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.yaml
diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.txt
b/Documentation/devicetree/bindings/phy/cdns,dphy.txt
deleted file mode 100644
index 1095bc4e72d9..
--- a/Documentation/devicetree/bindings/phy/cdns,dphy.txt
+++ /dev/null
@@ -1,20 +0,0 @@
-Cadence DPHY
-
-
-Cadence DPHY block.
-
-Required properties:
-- compatible: should be set to "cdns,dphy".
-- reg: physical base address and length of the DPHY registers.
-- clocks: DPHY reference clocks.
-- clock-names: must contain "psm" and "pll_ref".
-- #phy-cells: must be set to 0.
-
-Example:
- dphy0: dphy@fd0e{
- compatible = "cdns,dphy";
- reg = <0x0 0xfd0e 0x0 0x1000>;
- clocks = <_clk>, <_ref_clk>;
- clock-names = "psm", "pll_ref";
- #phy-cells = <0>;
- };
diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
new file mode 100644
index ..d1bbf96a8250
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/cdns,dphy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Cadence DPHY Device Tree Bindings
+
+maintainers:
+ - Pratyush Yadav
+
+properties:
+ compatible:
+items:
+ - const: cdns,dphy
+
+ reg:
+maxItems: 1
+description: Physical base address and length of the DPHY registers.
+
+ clocks:
+maxItems: 2
+description: DPHY reference clocks.
+
+ clock-names:
+items:
+ - const: psm
+ - const: pll_ref
+
+ "#phy-cells":
+const: 0
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - "#phy-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+
+dphy0: dphy@fd0e{
+compatible = "cdns,dphy";
+reg = <0xfd0e 0x1000>;
+clocks = <_clk>, <_ref_clk>;
+clock-names = "psm", "pll_ref";
+#phy-cells = <0>;
+};
--
2.30.0
[PATCH 13/16] media: ti-vpe: csi2rx: Add CSI2RX support
TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate
capture over a CSI-2 bus.
The Cadence CSI2RX IP acts as a bridge between the TI specific parts and
the CSI-2 protocol parts. TI then has a wrapper on top of this bridge
called the SHIM layer. It takes in data from stream 0, repacks it, and
sends it to memory over PSI-L DMA.
This driver acts as the "front end" to V4L2 client applications. It
implements the required ioctls and buffer operations, passes the
necessary calls on to the bridge, programs the SHIM layer, and performs
DMA via the dmaengine API to finally return the data to a buffer
supplied by the application.
Signed-off-by: Pratyush Yadav
---
MAINTAINERS | 7 +
drivers/media/platform/Kconfig| 11 +
drivers/media/platform/ti-vpe/Makefile| 1 +
drivers/media/platform/ti-vpe/ti-csi2rx.c | 964 ++
4 files changed, 983 insertions(+)
create mode 100644 drivers/media/platform/ti-vpe/ti-csi2rx.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 8c44fd8fd85d..06fb8fdb874d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18006,6 +18006,13 @@ S: Odd Fixes
F: drivers/clk/ti/
F: include/linux/clk/ti.h
+TI CSI2RX DRIVER
+M: Pratyush Yadav
+L: linux-me...@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/media/ti,csi2rx.yaml
+F: drivers/media/platform/ti-vpe/ti-csi2rx.c
+
TI DAVINCI MACHINE SUPPORT
M: Sekhar Nori
R: Bartosz Golaszewski
diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index b238a923d1b4..3b240d8e4cb8 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -200,6 +200,17 @@ config VIDEO_TI_CAL_MC
endif # VIDEO_TI_CAL
+config VIDEO_TI_CSI2RX
+ tristate "TI CSI2RX wrapper layer driver"
+ depends on VIDEO_DEV && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API &&
MEDIA_SUPPORT
+ depends on PHY_CADENCE_DPHY && VIDEO_CADENCE_CSI2RX
+ depends on ARCH_K3 || COMPILE_TEST
+ select VIDEOBUF2_DMA_CONTIG
+ select V4L2_FWNODE
+ help
+ Support for TI CSI2RX wrapper layer. This just enables the wrapper
driver.
+ The Cadence CSI2RX bridge driver needs to be enabled separately.
+
endif # V4L_PLATFORM_DRIVERS
menuconfig V4L_MEM2MEM_DRIVERS
diff --git a/drivers/media/platform/ti-vpe/Makefile
b/drivers/media/platform/ti-vpe/Makefile
index ad624056e039..c4ee49484b73 100644
--- a/drivers/media/platform/ti-vpe/Makefile
+++ b/drivers/media/platform/ti-vpe/Makefile
@@ -3,6 +3,7 @@ obj-$(CONFIG_VIDEO_TI_VPE) += ti-vpe.o
obj-$(CONFIG_VIDEO_TI_VPDMA) += ti-vpdma.o
obj-$(CONFIG_VIDEO_TI_SC) += ti-sc.o
obj-$(CONFIG_VIDEO_TI_CSC) += ti-csc.o
+obj-$(CONFIG_VIDEO_TI_CSI2RX) += ti-csi2rx.o
ti-vpe-y := vpe.o
ti-vpdma-y := vpdma.o
diff --git a/drivers/media/platform/ti-vpe/ti-csi2rx.c
b/drivers/media/platform/ti-vpe/ti-csi2rx.c
new file mode 100644
index ..355204ae473b
--- /dev/null
+++ b/drivers/media/platform/ti-vpe/ti-csi2rx.c
@@ -0,0 +1,964 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * TI CSI2 RX driver.
+ *
+ * Copyright (C) 2021 Texas Instruments Incorporated - https://www.ti.com/
+ *
+ * Author: Pratyush Yadav
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+#define TI_CSI2RX_MODULE_NAME "ti-csi2rx"
+
+#define SHIM_CNTL 0x10
+#define SHIM_CNTL_PIX_RST BIT(0)
+
+#define SHIM_DMACNTX 0x20
+#define SHIM_DMACNTX_ENBIT(31)
+#define SHIM_DMACNTX_YUV422GENMASK(27, 26)
+#define SHIM_DMACNTX_FMT GENMASK(5, 0)
+#define SHIM_DMACNTX_UYVY 0
+#define SHIM_DMACNTX_VYUY 1
+#define SHIM_DMACNTX_YUYV 2
+#define SHIM_DMACNTX_YVYU 3
+
+#define SHIM_PSI_CFG0 0x24
+#define SHIM_PSI_CFG0_SRC_TAG GENMASK(15, 0)
+#define SHIM_PSI_CFG0_DST_TAG GENMASK(31, 15)
+
+#define CSI_DF_YUV420 0x18
+#define CSI_DF_YUV422 0x1e
+#define CSI_DF_RGB444 0x20
+#define CSI_DF_RGB888 0x24
+
+struct ti_csi2rx_fmt {
+ u32 fourcc; /* Four character code. */
+ u32 code; /* Mbus code. */
+ u32 csi_df; /* CSI Data format. */
+ u8 bpp;/* Bits per pixel. */
+};
+
+struct ti_csi2rx_buffer {
+ /* Common v4l2 buffer. Must be first. */
+ struct vb2_v4l2_buffer vb;
+ struct list_headlist;
+};
+
+struct ti_csi2rx_dmaq {
+ struct list_headlist;
+};
+
+struct ti_csi2rx_dev {
+ struct device *dev;
+ void __iomem*shim;
+ struct v4l2_device v4l2_dev
[PATCH 12/16] dt-bindings: media: Add DT bindings for TI CSI2RX driver
TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate
capture over a CSI-2 bus. The TI CSI2RX platform driver glues all the
parts together.
Signed-off-by: Pratyush Yadav
---
.../devicetree/bindings/media/ti,csi2rx.yaml | 70 +++
1 file changed, 70 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/ti,csi2rx.yaml
diff --git a/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
new file mode 100644
index ..ebd894364391
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/ti,csi2rx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TI CSI2RX Wrapper Device Tree Bindings
+
+maintainers:
+ - Pratyush Yadav
+
+properties:
+ compatible:
+items:
+ - const: ti,csi2rx
+
+ dmas:
+description: RX DMA Channel 0
+
+ dma-names:
+items:
+ - const: rx0
+
+ reg:
+maxItems: 1
+description: Base address and size of the TI wrapper registers.
+
+ power-domains:
+maxItems: 1
+description:
+ PM domain provider node and an args specifier containing
+ the device id value.
+
+ ranges: true
+
+ "#address-cells":
+const: 2
+
+ "#size-cells":
+const: 2
+
+patternProperties:
+ "csi-bridge@":
+type: object
+description: CSI2 bridge node.
+
+required:
+ - compatible
+ - reg
+ - dmas
+ - dma-names
+ - power-domains
+ - "#address-cells"
+ - "#size-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+#include
+
+ti_csi2rx0: ticsi2rx {
+compatible = "ti,csi2rx";
+dmas = <_udmap 0x4940>;
+dma-names = "rx0";
+reg = <0x0 0x450 0x0 0x1000>;
+power-domains = <_pds 26 TI_SCI_PD_EXCLUSIVE>;
+#address-cells = <2>;
+#size-cells = <2>;
+};
--
2.30.0
[PATCH 11/16] dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX
The CSI2RX subsystem uses PSI-L DMA to transfer frames to memory. It can
have up to 32 threads but the current driver only supports using one. So
add an entry for that one thread.
Signed-off-by: Pratyush Yadav
---
drivers/dma/ti/k3-psil-j721e.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/dma/ti/k3-psil-j721e.c b/drivers/dma/ti/k3-psil-j721e.c
index 7580870ed746..19ffa31e6dc6 100644
--- a/drivers/dma/ti/k3-psil-j721e.c
+++ b/drivers/dma/ti/k3-psil-j721e.c
@@ -58,6 +58,14 @@
}, \
}
+#define PSIL_CSI2RX(x) \
+ { \
+ .thread_id = x, \
+ .ep_config = { \
+ .ep_type = PSIL_EP_NATIVE, \
+ }, \
+ }
+
/* PSI-L source thread IDs, used for RX (DMA_DEV_TO_MEM) */
static struct psil_ep j721e_src_ep_map[] = {
/* SA2UL */
@@ -138,6 +146,8 @@ static struct psil_ep j721e_src_ep_map[] = {
PSIL_PDMA_XY_PKT(0x4707),
PSIL_PDMA_XY_PKT(0x4708),
PSIL_PDMA_XY_PKT(0x4709),
+ /* CSI2RX */
+ PSIL_CSI2RX(0x4940),
/* CPSW9 */
PSIL_ETHERNET(0x4a00),
/* CPSW0 */
--
2.30.0
[PATCH 10/16] media: cadence: csi2rx: Add wrappers for subdev calls
When this bridge driver is being user by another platform driver, it
might want to call subdev operations like getting format, setting
format, enumerating format codes, etc. Add wrapper functions that pass
that call through to the sensor.
Currently wrappers are added only for the ops used by TI's platform
driver. More can be added later as needed.
Signed-off-by: Pratyush Yadav
---
drivers/media/platform/cadence/cdns-csi2rx.c | 77
1 file changed, 77 insertions(+)
diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c
b/drivers/media/platform/cadence/cdns-csi2rx.c
index 3385e1bc213e..2e8bbc53cb8b 100644
--- a/drivers/media/platform/cadence/cdns-csi2rx.c
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -408,12 +408,89 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev,
int enable)
return ret;
}
+static int csi2rx_g_frame_interval(struct v4l2_subdev *subdev,
+ struct v4l2_subdev_frame_interval *fi)
+{
+ struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
+
+ return v4l2_subdev_call(csi2rx->source_subdev, video, g_frame_interval,
+ fi);
+}
+
+static int csi2rx_s_frame_interval(struct v4l2_subdev *subdev,
+ struct v4l2_subdev_frame_interval *fi)
+{
+ struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
+
+ return v4l2_subdev_call(csi2rx->source_subdev, video, s_frame_interval,
+ fi);
+}
+
+static int csi2rx_enum_mbus_code(struct v4l2_subdev *subdev,
+struct v4l2_subdev_pad_config *cfg,
+struct v4l2_subdev_mbus_code_enum *code)
+{
+ struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
+
+ return v4l2_subdev_call(csi2rx->source_subdev, pad, enum_mbus_code,
+ cfg, code);
+}
+
+static int csi2rx_get_fmt(struct v4l2_subdev *subdev,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_format *fmt)
+{
+ struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
+
+ return v4l2_subdev_call(csi2rx->source_subdev, pad, get_fmt, cfg, fmt);
+}
+
+static int csi2rx_set_fmt(struct v4l2_subdev *subdev,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_format *fmt)
+{
+ struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
+
+ return v4l2_subdev_call(csi2rx->source_subdev, pad, set_fmt, cfg, fmt);
+}
+
+static int csi2rx_enum_frame_size(struct v4l2_subdev *subdev,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_frame_size_enum *fse)
+{
+ struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
+
+ return v4l2_subdev_call(csi2rx->source_subdev, pad, enum_frame_size,
+ cfg, fse);
+}
+
+static int csi2rx_enum_frame_interval(struct v4l2_subdev *subdev,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_frame_interval_enum
*fie)
+{
+ struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
+
+ return v4l2_subdev_call(csi2rx->source_subdev, pad, enum_frame_interval,
+ cfg, fie);
+}
+
static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
.s_stream = csi2rx_s_stream,
+ .g_frame_interval = csi2rx_g_frame_interval,
+ .s_frame_interval = csi2rx_s_frame_interval,
+};
+
+static const struct v4l2_subdev_pad_ops csi2rx_pad_ops = {
+ .enum_mbus_code = csi2rx_enum_mbus_code,
+ .get_fmt= csi2rx_get_fmt,
+ .set_fmt= csi2rx_set_fmt,
+ .enum_frame_size = csi2rx_enum_frame_size,
+ .enum_frame_interval = csi2rx_enum_frame_interval,
};
static const struct v4l2_subdev_ops csi2rx_subdev_ops = {
.video = _video_ops,
+ .pad= _pad_ops,
};
static int csi2rx_async_bound(struct v4l2_async_notifier *notifier,
--
2.30.0
[PATCH 09/16] media: cadence: csi2rx: Turn subdev power on before starting stream
The subdevice power needs to be turned on before the stream is started.
Otherwise it might not be in the proper state to stream the data. Turn
it off when stopping the stream.
Signed-off-by: Pratyush Yadav
---
drivers/media/platform/cadence/cdns-csi2rx.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c
b/drivers/media/platform/cadence/cdns-csi2rx.c
index 7d1ac51e0698..3385e1bc213e 100644
--- a/drivers/media/platform/cadence/cdns-csi2rx.c
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -256,6 +256,10 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
writel(reg, csi2rx->base + CSI2RX_STATIC_CFG_REG);
+ ret = v4l2_subdev_call(csi2rx->source_subdev, core, s_power, true);
+ if (ret && ret != -ENOIOCTLCMD)
+ goto err_disable_pclk;
+
ret = v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, true);
if (ret)
goto err_disable_pclk;
@@ -358,6 +362,10 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
if (v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, false))
dev_warn(csi2rx->dev, "Couldn't disable our subdev\n");
+ ret = v4l2_subdev_call(csi2rx->source_subdev, core, s_power, false);
+ if (ret && ret != -ENOIOCTLCMD)
+ dev_warn(csi2rx->dev, "Couldn't power off subdev\n");
+
if (csi2rx->dphy) {
writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG);
--
2.30.0
[PATCH 08/16] media: cadence: csi2rx: Fix stream data configuration
Firstly, there is no VC_EN bit present in the STREAM_DATA_CFG register. Bit 31 is part of the VL_SELECT field. Remove it completely. Secondly, it makes little sense to enable ith virtual channel for ith stream. Sure, there might be a use-case that demands it. But there might also be a use case that demands all streams to use the 0th virtual channel. Prefer this case over the former because it is less arbitrary and also makes it very clear what the limitations of the current driver is instead of giving a false impression that multiple virtual channels are supported. Signed-off-by: Pratyush Yadav --- drivers/media/platform/cadence/cdns-csi2rx.c | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c index eca65b157f59..7d1ac51e0698 100644 --- a/drivers/media/platform/cadence/cdns-csi2rx.c +++ b/drivers/media/platform/cadence/cdns-csi2rx.c @@ -48,7 +48,6 @@ #define CSI2RX_STREAM_STATUS_RDY BIT(31) #define CSI2RX_STREAM_DATA_CFG_REG(n) (CSI2RX_STREAM_BASE(n) + 0x008) -#define CSI2RX_STREAM_DATA_CFG_EN_VC_SELECTBIT(31) #define CSI2RX_STREAM_DATA_CFG_VC_SELECT(n)BIT((n) + 16) #define CSI2RX_STREAM_CFG_REG(n) (CSI2RX_STREAM_BASE(n) + 0x00c) @@ -290,8 +289,11 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF, csi2rx->base + CSI2RX_STREAM_CFG_REG(i)); - writel(CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT | - CSI2RX_STREAM_DATA_CFG_VC_SELECT(i), + /* +* Enable one virtual channel. When multiple virtual channels +* are supported this will have to be changed. +*/ + writel(CSI2RX_STREAM_DATA_CFG_VC_SELECT(0), csi2rx->base + CSI2RX_STREAM_DATA_CFG_REG(i)); writel(CSI2RX_STREAM_CTRL_START, -- 2.30.0
[PATCH 07/16] media: cadence: csi2rx: Set the STOP bit when stopping a stream
The stream stop procedure says that the STOP bit should be set when the
stream is to be stopped, and then the ready bit in stream status
register polled to make sure the STOP operation is finished.
Signed-off-by: Pratyush Yadav
---
drivers/media/platform/cadence/cdns-csi2rx.c | 18 +-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c
b/drivers/media/platform/cadence/cdns-csi2rx.c
index b03d2d2e6762..eca65b157f59 100644
--- a/drivers/media/platform/cadence/cdns-csi2rx.c
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -8,6 +8,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -40,8 +41,12 @@
#define CSI2RX_STREAM_CTRL_REG(n) (CSI2RX_STREAM_BASE(n) + 0x000)
#define CSI2RX_STREAM_CTRL_SOFT_RSTBIT(4)
+#define CSI2RX_STREAM_CTRL_STOPBIT(1)
#define CSI2RX_STREAM_CTRL_START BIT(0)
+#define CSI2RX_STREAM_STATUS_REG(n)(CSI2RX_STREAM_BASE(n) + 0x004)
+#define CSI2RX_STREAM_STATUS_RDY BIT(31)
+
#define CSI2RX_STREAM_DATA_CFG_REG(n) (CSI2RX_STREAM_BASE(n) + 0x008)
#define CSI2RX_STREAM_DATA_CFG_EN_VC_SELECTBIT(31)
#define CSI2RX_STREAM_DATA_CFG_VC_SELECT(n)BIT((n) + 16)
@@ -325,12 +330,23 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
static void csi2rx_stop(struct csi2rx_priv *csi2rx)
{
unsigned int i;
+ u32 val;
+ int ret;
clk_prepare_enable(csi2rx->p_clk);
clk_disable_unprepare(csi2rx->sys_clk);
for (i = 0; i < csi2rx->max_streams; i++) {
- writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
+ writel(CSI2RX_STREAM_CTRL_STOP,
+ csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
+
+ ret = readl_relaxed_poll_timeout(csi2rx->base +
+CSI2RX_STREAM_STATUS_REG(i),
+val,
+(val &
CSI2RX_STREAM_STATUS_RDY),
+10, 1);
+ if (ret)
+ dev_warn(csi2rx->dev, "Failed to stop stream%d\n", i);
clk_disable_unprepare(csi2rx->pixel_clk[i]);
}
--
2.30.0
[PATCH 05/16] media: cadence: csi2rx: Add external DPHY support
Some platforms like TI's J721E can have the CSI2RX paired with an
external DPHY. Add support to enable and configure the DPHY using the
generic PHY framework.
Get the pixel rate and bpp from the subdev and pass them on to the DPHY
along with the number of lanes. All other settings are left to their
default values.
Signed-off-by: Pratyush Yadav
---
drivers/media/platform/cadence/cdns-csi2rx.c | 147 +--
1 file changed, 137 insertions(+), 10 deletions(-)
diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c
b/drivers/media/platform/cadence/cdns-csi2rx.c
index c68a3eac62cd..31bd80e3f780 100644
--- a/drivers/media/platform/cadence/cdns-csi2rx.c
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -30,6 +30,12 @@
#define CSI2RX_STATIC_CFG_DLANE_MAP(llane, plane) ((plane) << (16 +
(llane) * 4))
#define CSI2RX_STATIC_CFG_LANES_MASK GENMASK(11, 8)
+#define CSI2RX_DPHY_LANE_CTRL_REG 0x40
+#define CSI2RX_DPHY_CL_RST BIT(16)
+#define CSI2RX_DPHY_DL_RST(i) BIT((i) + 12)
+#define CSI2RX_DPHY_CL_EN BIT(4)
+#define CSI2RX_DPHY_DL_EN(i) BIT(i)
+
#define CSI2RX_STREAM_BASE(n) (((n) + 1) * 0x100)
#define CSI2RX_STREAM_CTRL_REG(n) (CSI2RX_STREAM_BASE(n) + 0x000)
@@ -54,6 +60,11 @@ enum csi2rx_pads {
CSI2RX_PAD_MAX,
};
+struct csi2rx_fmt {
+ u32 code;
+ u8 bpp;
+};
+
struct csi2rx_priv {
struct device *dev;
unsigned intcount;
@@ -85,6 +96,52 @@ struct csi2rx_priv {
int source_pad;
};
+static const struct csi2rx_fmt formats[] = {
+ {
+ .code = MEDIA_BUS_FMT_YUYV8_2X8,
+ .bpp= 16,
+ },
+ {
+ .code = MEDIA_BUS_FMT_UYVY8_2X8,
+ .bpp= 16,
+ },
+ {
+ .code = MEDIA_BUS_FMT_YVYU8_2X8,
+ .bpp= 16,
+ },
+ {
+ .code = MEDIA_BUS_FMT_VYUY8_2X8,
+ .bpp= 16,
+ },
+};
+
+static u8 csi2rx_get_bpp(u32 code)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(formats); i++) {
+ if (formats[i].code == code)
+ return formats[i].bpp;
+ }
+
+ return 0;
+}
+
+static s64 csi2rx_get_pixel_rate(struct csi2rx_priv *csi2rx)
+{
+ struct v4l2_ctrl *ctrl;
+
+ ctrl = v4l2_ctrl_find(csi2rx->source_subdev->ctrl_handler,
+ V4L2_CID_PIXEL_RATE);
+ if (!ctrl) {
+ dev_err(csi2rx->dev, "no pixel rate control in subdev: %s\n",
+ csi2rx->source_subdev->name);
+ return -EINVAL;
+ }
+
+ return v4l2_ctrl_g_ctrl_int64(ctrl);
+}
+
static inline
struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev)
{
@@ -101,6 +158,55 @@ static void csi2rx_reset(struct csi2rx_priv *csi2rx)
writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG);
}
+static int csi2rx_configure_external_dphy(struct csi2rx_priv *csi2rx)
+{
+ union phy_configure_opts opts = { };
+ struct phy_configure_opts_mipi_dphy *cfg = _dphy;
+ struct v4l2_subdev_format sd_fmt;
+ s64 pixel_rate;
+ int ret;
+ u8 bpp;
+
+ sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+ sd_fmt.pad = 0;
+
+ ret = v4l2_subdev_call(csi2rx->source_subdev, pad, get_fmt, NULL,
+ _fmt);
+ if (ret)
+ return ret;
+
+ bpp = csi2rx_get_bpp(sd_fmt.format.code);
+ if (!bpp)
+ return -EINVAL;
+
+ pixel_rate = csi2rx_get_pixel_rate(csi2rx);
+ if (pixel_rate < 0)
+ return pixel_rate;
+
+ ret = phy_mipi_dphy_get_default_config(pixel_rate, bpp,
+ csi2rx->num_lanes, cfg);
+ if (ret)
+ return ret;
+
+ ret = phy_set_mode_ext(csi2rx->dphy, PHY_MODE_MIPI_DPHY,
+ PHY_MIPI_DPHY_SUBMODE_RX);
+ if (ret)
+ return ret;
+
+ ret = phy_power_on(csi2rx->dphy);
+ if (ret)
+ return ret;
+
+ ret = phy_configure(csi2rx->dphy, );
+ if (ret) {
+ /* Can't do anything if it fails. Ignore the return value. */
+ phy_power_off(csi2rx->dphy);
+ return ret;
+ }
+
+ return 0;
+}
+
static int csi2rx_start(struct csi2rx_priv *csi2rx)
{
unsigned int i;
@@ -139,6 +245,17 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
if (ret)
goto err_disable_pclk;
+ /* Enable DPHY clk and data lanes. */
+ if (csi2rx->dphy) {
+ reg = CSI2RX_DPHY_CL_EN | CSI2RX_DPHY_CL_RST;
+ for (i = 0; i < csi2rx->
[PATCH 06/16] media: cadence: csi2rx: Soft reset the streams before starting capture
This resets the stream state machines and FIFOs, giving them a clean
slate. On J721E if the streams are not reset before starting the
capture, the captured frame gets wrapped around vertically on every run
after the first.
Signed-off-by: Pratyush Yadav
---
drivers/media/platform/cadence/cdns-csi2rx.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c
b/drivers/media/platform/cadence/cdns-csi2rx.c
index 31bd80e3f780..b03d2d2e6762 100644
--- a/drivers/media/platform/cadence/cdns-csi2rx.c
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -39,6 +39,7 @@
#define CSI2RX_STREAM_BASE(n) (((n) + 1) * 0x100)
#define CSI2RX_STREAM_CTRL_REG(n) (CSI2RX_STREAM_BASE(n) + 0x000)
+#define CSI2RX_STREAM_CTRL_SOFT_RSTBIT(4)
#define CSI2RX_STREAM_CTRL_START BIT(0)
#define CSI2RX_STREAM_DATA_CFG_REG(n) (CSI2RX_STREAM_BASE(n) + 0x008)
@@ -150,12 +151,22 @@ struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct
v4l2_subdev *subdev)
static void csi2rx_reset(struct csi2rx_priv *csi2rx)
{
+ int i;
+
writel(CSI2RX_SOFT_RESET_PROTOCOL | CSI2RX_SOFT_RESET_FRONT,
csi2rx->base + CSI2RX_SOFT_RESET_REG);
udelay(10);
writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG);
+
+ /* Reset individual streams. */
+ for (i = 0; i < csi2rx->max_streams; i++) {
+ writel(CSI2RX_STREAM_CTRL_SOFT_RST,
+ csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
+ usleep_range(10, 20);
+ writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
+ }
}
static int csi2rx_configure_external_dphy(struct csi2rx_priv *csi2rx)
--
2.30.0
[PATCH 04/16] phy: cdns-dphy: Add Rx support
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 PHY is in Tx mode by default and it needs to be set in Rx mode by
setting the submode to PHY_MIPI_DPHY_SUBMODE_RX in the set_mode()
callback.
Signed-off-by: Pratyush Yadav
---
drivers/phy/cadence/cdns-dphy.c | 237
1 file changed, 237 insertions(+)
diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
index 7d5f7b333893..7bbca679e2bb 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
+#include
#include
#include
+#include
#include
#include
#include
@@ -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_ENBIT(0)
+#define DPHY_CMN_RX_BANDGAP_TIMER_MASK GENMASK(8, 1)
#define DPHY_CMN_TX_MODE_ENBIT(9)
+#define DPHY_CMN_RX_MODE_ENBIT(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_REGBIT(6)
#define DPHY_CMN_OPDIV(x) ((x) << 7)
+#define DPHY_BAND_CFG DPHY_PCS(0x0)
+#define DPHY_BAND_CFG_LEFT_BANDGENMASK(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 0x
+#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_L0DPHY_ISO(0x14)
+#define DPHY_ISO_DL_CTRL_L1DPHY_ISO(0x20)
+#define DPHY_ISO_DL_CTRL_L2DPHY_ISO(0x30)
+#define DPHY_ISO_DL_CTRL_L3DPHY_ISO(0x3c)
+#define DPHY_ISO_LANE_READY_BIT0
+#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_OVERHEAD6
#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;
@@ -312,6 +339,214 @@ static const struct cdns_dphy_ops tx_ref_dphy_ops = {
.set_psm_div = cdns_dphy_ref_set_psm_div,
};
+static int cdns_dphy_rx_power_on(struct cdns_dphy *dphy)
+{
+ /* 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 cdns_dphy *dphy)
+{
+ 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 = hs_clk_rate / 100UL;
+
+ if (rate < 80 || rate >= 2500)
+ return -EOPNOTSUPP;
+
+ if (rate >= 80 && rate < 100)
+ return 0;
+
+ if (rate >= 100 && rate < 120)
+ return 1;
+
+ if (rate >= 120 && rate < 160)
+ return 2;
+
+ if (rate >= 160 && rate < 200)
+ return 3;
+
+ if (rate >= 200 && rate < 240)
+ return 4;
+
+ if (rate >= 240 && rate < 280)
+ return 5;
+
+ if (rate >= 280 && rate < 320)
+ return 6;
+
+ if (rate >= 320 && rate < 360)
+ return 7;
+
+ if (rate >= 360 && rate < 400)
+ return 8;
+
+ if (rate >= 400 && rate < 480)
+ return 9;
+
+ if (rate >= 480 && rate < 560)
+ return 10;
+
+ if (rate >= 560 && rate < 640)
+ return 11;
+
+ if (rate >= 640 && rate < 720)
+ return 12;
+
+ i
[PATCH 00/16] CSI2RX support on J721E
Hi, This series adds support for CSI2 capture on J721E. It includes some fixes to the Cadence CSI2RX driver, adds Rx support to Cadence DPHY driver, and finally adds the TI CSI2RX wrapper driver. Tested on TI's J721E with OV5640 sensor. Paul Kocialkowski (1): phy: Distinguish between Rx and Tx for MIPI D-PHY with submodes Pratyush Yadav (15): phy: cdns-dphy: Prepare for Rx support phy: cdns-dphy: Allow setting mode phy: cdns-dphy: Add Rx support media: cadence: csi2rx: Add external DPHY support media: cadence: csi2rx: Soft reset the streams before starting capture media: cadence: csi2rx: Set the STOP bit when stopping a stream media: cadence: csi2rx: Fix stream data configuration media: cadence: csi2rx: Turn subdev power on before starting stream media: cadence: csi2rx: Add wrappers for subdev calls dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX dt-bindings: media: Add DT bindings for TI CSI2RX driver media: ti-vpe: csi2rx: Add CSI2RX support dt-bindings: phy: Convert Cadence DPHY binding to YAML dt-bindings: phy: cdns,dphy: make clocks optional dt-bindings: phy: cdns,dphy: add power-domains property .../devicetree/bindings/media/ti,csi2rx.yaml | 70 ++ .../devicetree/bindings/phy/cdns,dphy.txt | 20 - .../devicetree/bindings/phy/cdns,dphy.yaml| 52 + MAINTAINERS | 7 + drivers/dma/ti/k3-psil-j721e.c| 10 + drivers/media/platform/Kconfig| 11 + drivers/media/platform/cadence/cdns-csi2rx.c | 269 - drivers/media/platform/ti-vpe/Makefile| 1 + drivers/media/platform/ti-vpe/ti-csi2rx.c | 964 ++ drivers/phy/cadence/cdns-dphy.c | 407 +++- include/linux/phy/phy-mipi-dphy.h | 13 + 11 files changed, 1754 insertions(+), 70 deletions(-) create mode 100644 Documentation/devicetree/bindings/media/ti,csi2rx.yaml delete mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.txt create mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.yaml create mode 100644 drivers/media/platform/ti-vpe/ti-csi2rx.c -- 2.30.0
[PATCH 03/16] phy: cdns-dphy: Allow setting mode
Allow callers to set the PHY mode. The main mode should always be
PHY_MODE_MIPI_DPHY but the submode can either be
PHY_MIPI_DPHY_SUBMODE_RX or PHY_MIPI_DPHY_SUBMODE_TX. Update the ops
based on the requested submode.
Signed-off-by: Pratyush Yadav
---
drivers/phy/cadence/cdns-dphy.c | 30 ++
1 file changed, 30 insertions(+)
diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
index 8656f2102a91..7d5f7b333893 100644
--- a/drivers/phy/cadence/cdns-dphy.c
+++ b/drivers/phy/cadence/cdns-dphy.c
@@ -365,11 +365,41 @@ static int cdns_dphy_configure(struct phy *phy, union
phy_configure_opts *opts)
return 0;
}
+static int cdns_dphy_set_mode(struct phy *phy, enum phy_mode mode, int submode)
+{
+ struct cdns_dphy *dphy = phy_get_drvdata(phy);
+ const struct cdns_dphy_driver_data *ddata;
+
+ ddata = of_device_get_match_data(dphy->dev);
+ if (!ddata)
+ return -EINVAL;
+
+ if (mode != PHY_MODE_MIPI_DPHY)
+ return -EINVAL;
+
+ if (submode == PHY_MIPI_DPHY_SUBMODE_TX) {
+ if (!ddata->tx)
+ return -EOPNOTSUPP;
+
+ dphy->ops = ddata->tx;
+ } else if (submode == PHY_MIPI_DPHY_SUBMODE_RX) {
+ if (!ddata->rx)
+ return -EOPNOTSUPP;
+
+ dphy->ops = ddata->rx;
+ } else {
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
static const struct phy_ops cdns_dphy_ops = {
.configure = cdns_dphy_configure,
.validate = cdns_dphy_validate,
.power_on = cdns_dphy_power_on,
.power_off = cdns_dphy_power_off,
+ .set_mode = cdns_dphy_set_mode,
};
static int cdns_dphy_probe(struct platform_device *pdev)
--
2.30.0
[PATCH 02/16] phy: cdns-dphy: Prepare for Rx support
The Rx programming sequence differs from the Tx programming sequence.
Currently only Tx mode is supported. Move all the Tx related parts into
a set of Tx-specific hooks that are then called by the main PHY
framework hooks. This way when Rx support is added all that is needed to
be done is to plug in the Rx hooks.
The clocks "psm" and "pll_ref" are not used by the Rx path so make them
optional in the probe and then check if they exist in the power_on()
hook.
Signed-off-by: Pratyush Yadav
---
drivers/phy/cadence/cdns-dphy.c | 140
1 file changed, 104 insertions(+), 36 deletions(-)
diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
index ba042e39cfaf..8656f2102a91 100644
--- a/drivers/phy/cadence/cdns-dphy.c
+++ b/drivers/phy/cadence/cdns-dphy.c
@@ -75,6 +75,11 @@ struct cdns_dphy;
struct cdns_dphy_ops {
int (*probe)(struct cdns_dphy *dphy);
void (*remove)(struct cdns_dphy *dphy);
+ int (*power_on)(struct cdns_dphy *dphy);
+ int (*power_off)(struct cdns_dphy *dphy);
+ int (*validate)(struct cdns_dphy *dphy, enum phy_mode mode, int submode,
+ union phy_configure_opts *opts);
+ int (*configure)(struct cdns_dphy *dphy, union phy_configure_opts
*opts);
void (*set_psm_div)(struct cdns_dphy *dphy, u8 div);
void (*set_clk_lane_cfg)(struct cdns_dphy *dphy,
enum cdns_dphy_clk_lane_cfg cfg);
@@ -86,12 +91,18 @@ struct cdns_dphy_ops {
struct cdns_dphy {
struct cdns_dphy_cfg cfg;
void __iomem *regs;
+ struct device *dev;
struct clk *psm_clk;
struct clk *pll_ref_clk;
const struct cdns_dphy_ops *ops;
struct phy *phy;
};
+struct cdns_dphy_driver_data {
+ const struct cdns_dphy_ops *tx;
+ const struct cdns_dphy_ops *rx;
+};
+
static int cdns_dsi_get_dphy_pll_cfg(struct cdns_dphy *dphy,
struct cdns_dphy_cfg *cfg,
struct phy_configure_opts_mipi_dphy *opts,
@@ -199,20 +210,9 @@ static void cdns_dphy_ref_set_psm_div(struct cdns_dphy
*dphy, u8 div)
dphy->regs + DPHY_PSM_CFG);
}
-/*
- * This is the reference implementation of DPHY hooks. Specific integration of
- * this IP may have to re-implement some of them depending on how they decided
- * to wire things in the SoC.
- */
-static const struct cdns_dphy_ops ref_dphy_ops = {
- .get_wakeup_time_ns = cdns_dphy_ref_get_wakeup_time_ns,
- .set_pll_cfg = cdns_dphy_ref_set_pll_cfg,
- .set_psm_div = cdns_dphy_ref_set_psm_div,
-};
-
-static int cdns_dphy_config_from_opts(struct phy *phy,
- struct phy_configure_opts_mipi_dphy *opts,
- struct cdns_dphy_cfg *cfg)
+static int cdns_dphy_tx_config_from_opts(struct phy *phy,
+struct phy_configure_opts_mipi_dphy
*opts,
+struct cdns_dphy_cfg *cfg)
{
struct cdns_dphy *dphy = phy_get_drvdata(phy);
unsigned int dsi_hfp_ext = 0;
@@ -232,24 +232,13 @@ static int cdns_dphy_config_from_opts(struct phy *phy,
return 0;
}
-static int cdns_dphy_validate(struct phy *phy, enum phy_mode mode, int submode,
- union phy_configure_opts *opts)
+static int cdns_dphy_tx_configure(struct cdns_dphy *dphy,
+ union phy_configure_opts *opts)
{
struct cdns_dphy_cfg cfg = { 0 };
-
- if (mode != PHY_MODE_MIPI_DPHY)
- return -EINVAL;
-
- return cdns_dphy_config_from_opts(phy, >mipi_dphy, );
-}
-
-static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
-{
- struct cdns_dphy *dphy = phy_get_drvdata(phy);
- struct cdns_dphy_cfg cfg = { 0 };
int ret;
- ret = cdns_dphy_config_from_opts(phy, >mipi_dphy, );
+ ret = cdns_dphy_tx_config_from_opts(dphy->phy, >mipi_dphy, );
if (ret)
return ret;
@@ -279,9 +268,21 @@ static int cdns_dphy_configure(struct phy *phy, union
phy_configure_opts *opts)
return 0;
}
-static int cdns_dphy_power_on(struct phy *phy)
+static int cdns_dphy_tx_validate(struct cdns_dphy *dphy, enum phy_mode mode,
+int submode, union phy_configure_opts *opts)
{
- struct cdns_dphy *dphy = phy_get_drvdata(phy);
+ struct cdns_dphy_cfg cfg = { 0 };
+
+ if (submode != PHY_MIPI_DPHY_SUBMODE_TX)
+ return -EINVAL;
+
+ return cdns_dphy_tx_config_from_opts(dphy->phy, >mipi_dphy, );
+}
+
+static int cdns_dphy_tx_power_on(struct cdns_dphy *dphy)
+{
+ if (!dphy->psm_clk || !dphy->pll_ref_clk)
+ return -EINVAL;
clk_prepare_enable(dphy->psm_clk);
clk_prepare_enable(dphy->pll_ref_clk);
@@ -293,16 +29
[PATCH 01/16] phy: Distinguish between Rx and Tx for MIPI D-PHY with submodes
From: Paul Kocialkowski
As some D-PHY controllers support both Rx and Tx mode, we need a way for
users to explicitly request one or the other. For instance, Rx mode can
be used along with MIPI CSI-2 while Tx mode can be used with MIPI DSI.
Introduce new MIPI D-PHY PHY submodes to use with PHY_MODE_MIPI_DPHY.
The default (zero value) is kept to Tx so only the rkisp1 driver, which
uses D-PHY in Rx mode, needs to be adapted.
Signed-off-by: Paul Kocialkowski
Signed-off-by: Pratyush Yadav
---
include/linux/phy/phy-mipi-dphy.h | 13 +
1 file changed, 13 insertions(+)
diff --git a/include/linux/phy/phy-mipi-dphy.h
b/include/linux/phy/phy-mipi-dphy.h
index a877ffee845d..0f57ef46a8b5 100644
--- a/include/linux/phy/phy-mipi-dphy.h
+++ b/include/linux/phy/phy-mipi-dphy.h
@@ -6,6 +6,19 @@
#ifndef __PHY_MIPI_DPHY_H_
#define __PHY_MIPI_DPHY_H_
+/**
+ * enum phy_mipi_dphy_submode - MIPI D-PHY sub-mode
+ *
+ * A MIPI D-PHY can be used to transmit or receive data.
+ * Since some controllers can support both, the direction to enable is
specified
+ * with the PHY sub-mode. Transmit is assumed by default with phy_set_mode.
+ */
+
+enum phy_mipi_dphy_submode {
+ PHY_MIPI_DPHY_SUBMODE_TX = 0,
+ PHY_MIPI_DPHY_SUBMODE_RX,
+};
+
/**
* struct phy_configure_opts_mipi_dphy - MIPI D-PHY configuration set
*
--
2.30.0
Re: [PATCH v2 02/13] spi: cadence-quadspi: Add QSPI support for Pensando Elba SoC
On 28/03/21 06:59PM, Brad Larson wrote:
> Add QSPI controller support for Pensando Elba SoC
>
> Signed-off-by: Brad Larson
> ---
> drivers/spi/spi-cadence-quadspi.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/spi/spi-cadence-quadspi.c
> b/drivers/spi/spi-cadence-quadspi.c
> index 52ddb3255d88..4aacb0b2dbc7 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -1353,6 +1353,7 @@ static int cqspi_request_mmap_dma(struct cqspi_st
> *cqspi)
> cqspi->rx_chan = dma_request_chan_by_mask();
> if (IS_ERR(cqspi->rx_chan)) {
> int ret = PTR_ERR(cqspi->rx_chan);
> +
Unrelated whitespace change. Please move it into a separate cleanup
patch.
> cqspi->rx_chan = NULL;
> return dev_err_probe(>pdev->dev, ret, "No Rx DMA
> available\n");
> }
> @@ -1633,6 +1634,10 @@ static const struct cqspi_driver_platdata
> intel_lgm_qspi = {
> .quirks = CQSPI_DISABLE_DAC_MODE,
> };
>
> +static const struct cqspi_driver_platdata pen_cdns_qspi = {
> + .quirks = CQSPI_NEEDS_WR_DELAY | CQSPI_DISABLE_DAC_MODE,
> +};
> +
> static const struct of_device_id cqspi_dt_ids[] = {
> {
> .compatible = "cdns,qspi-nor",
> @@ -1650,6 +1655,10 @@ static const struct of_device_id cqspi_dt_ids[] = {
> .compatible = "intel,lgm-qspi",
> .data = _lgm_qspi,
> },
> + {
> + .compatible = "pensando,cdns-qspi",
> + .data = _cdns_qspi,
> + },
> { /* end of table */ }
> };
>
> --
> 2.17.1
>
Rest of the patch looks good to me.
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [PATCH v2 10/13] dt-bindings: spi: cadence-qspi: Add support for Pensando Elba SoC
a-schemas/core.yaml# > + > +title: Cadence Quad SPI controller > + > +maintainers: > + - Ramuthevar Vadivel Murugan > + - Brad Larson > + > +properties: > + compatible: > +contains: > + enum: > +- cdns,qspi-nor # Generic default > +- ti,k2g-qspi # TI 66AK2G SoC > +- ti,am654-ospi # TI AM654 SoC > +- intel,lgm-qspi # Intel LGM SoC > +- pensando,cdns-qspi # Pensando Elba SoC Wouldn't this allow any combination of all 5 strings? So for example this would allow "ti,am654-ospi", "pensando,cdns-qspi" which is obviously not correct. I sent a patch recently [0] that does this correctly and it has gotten Rob's blessing. So I suggest you build your patch on top of that. [...] [0] https://patchwork.kernel.org/project/spi-devel-general/patch/20210326130034.15231-5-p.ya...@ti.com/ -- Regards, Pratyush Yadav Texas Instruments Inc.
Re: [PATCH 4/4] dt-bindings: spi: Convert cadence-quadspi.txt to cadence-quadspi.yaml
On 27/03/21 12:36PM, Rob Herring wrote: > On Fri, Mar 26, 2021 at 06:30:34PM +0530, Pratyush Yadav wrote: > > From: Ramuthevar Vadivel Murugan > > > > > > There is no way as of now to have a parent or bus defining properties > > for child nodes. For now, avoid it in the example to silence warnings on > > dt_schema_check. We can figure out how to deal with actual dts files > > later. > > > > Signed-off-by: Ramuthevar Vadivel Murugan > > > > Signed-off-by: Pratyush Yadav > > [p.ya...@ti.com: Fix how compatible is defined, make reset optional, fix > > minor typos, remove subnode properties in example, update commit > > message.] > > --- > > .../bindings/spi/cadence-quadspi.txt | 68 - > > .../bindings/spi/cdns,qspi-nor.yaml | 143 ++ > > 2 files changed, 143 insertions(+), 68 deletions(-) > > delete mode 100644 > > Documentation/devicetree/bindings/spi/cadence-quadspi.txt > > create mode 100644 Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml > > > > diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml > > b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml > > new file mode 100644 > > index ..0e7087cc8bf9 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml > > @@ -0,0 +1,143 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/spi/cdns,qspi-nor.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Cadence Quad SPI controller > > + > > +maintainers: > > + - Pratyush Yadav > > + > > +allOf: > > + - $ref: spi-controller.yaml# > > + > > +properties: > > + compatible: > > +oneOf: > > + - items: > > + - enum: > > + - ti,k2g-qspi > > + - ti,am654-ospi > > + - intel,lgm-qspi > > + - const: cdns,qspi-nor > > + - const: cdns,qspi-nor > > + > > + reg: > > +items: > > + - description: the controller register set > > + - description: the controller data area > > + > > + interrupts: > > +maxItems: 1 > > + > > + clocks: > > +maxItems: 1 > > + > > + cdns,fifo-depth: > > +description: > > + Size of the data FIFO in words. > > +$ref: "/schemas/types.yaml#/definitions/uint32" > > +enum: [ 128, 256 ] > > +default: 128 > > + > > + cdns,fifo-width: > > +$ref: /schemas/types.yaml#/definitions/uint32 > > +description: > > + Bus width of the data FIFO in bytes. > > +default: 4 > > I assume there's some constraints on this? IIUC this is a matter of how the peripheral is implemented and there are no clear constraints. Different implementations can use different bus widths for the SRAM bus but I don't see any mention of minimum or maximum values. FWIW, all users in the kernel use a 4 byte bus. > > With that, > > Reviewed-by: Rob Herring Thanks. > > > + > > + cdns,trigger-address: > > +$ref: /schemas/types.yaml#/definitions/uint32 > > +description: > > + 32-bit indirect AHB trigger address. > > + > > + cdns,is-decoded-cs: > > +type: boolean > > +description: > > + Flag to indicate whether decoder is used to select different chip > > select > > + for different memory regions. > > + > > + cdns,rclk-en: > > +type: boolean > > +description: > > + Flag to indicate that QSPI return clock is used to latch the read > > + data rather than the QSPI clock. Make sure that QSPI return clock > > + is populated on the board before using this property. > > + > > + resets: > > +maxItems: 2 > > + > > + reset-names: > > +minItems: 1 > > +maxItems: 2 > > +items: > > + enum: [ qspi, qspi-ocp ] > > + > > +# subnode's properties > > +patternProperties: > > + "@[0-9a-f]+$": > > +type: object > > +description: > > + Flash device uses the below defined properties in the subnode. > > + > > +properties: > > + cdns,read-delay: > > +$ref: /schemas/types.yaml#/definitions/uint32 > > +description: > > + Delay for read capture logic, in clock cycles. > > + > > + cdns,tshsl-ns: > > +d
[PATCH 4/4] dt-bindings: spi: Convert cadence-quadspi.txt to cadence-quadspi.yaml
From: Ramuthevar Vadivel Murugan
There is no way as of now to have a parent or bus defining properties
for child nodes. For now, avoid it in the example to silence warnings on
dt_schema_check. We can figure out how to deal with actual dts files
later.
Signed-off-by: Ramuthevar Vadivel Murugan
Signed-off-by: Pratyush Yadav
[p.ya...@ti.com: Fix how compatible is defined, make reset optional, fix
minor typos, remove subnode properties in example, update commit
message.]
---
.../bindings/spi/cadence-quadspi.txt | 68 -
.../bindings/spi/cdns,qspi-nor.yaml | 143 ++
2 files changed, 143 insertions(+), 68 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/spi/cadence-quadspi.txt
create mode 100644 Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
diff --git a/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
b/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
deleted file mode 100644
index 8ace832a2d80..
--- a/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
+++ /dev/null
@@ -1,68 +0,0 @@
-* Cadence Quad SPI controller
-
-Required properties:
-- compatible : should be one of the following:
- Generic default - "cdns,qspi-nor".
- For TI 66AK2G SoC - "ti,k2g-qspi", "cdns,qspi-nor".
- For TI AM654 SoC - "ti,am654-ospi", "cdns,qspi-nor".
- For Intel LGM SoC - "intel,lgm-qspi", "cdns,qspi-nor".
-- reg : Contains two entries, each of which is a tuple consisting of a
- physical address and length. The first entry is the address and
- length of the controller register set. The second entry is the
- address and length of the QSPI Controller data area.
-- interrupts : Unit interrupt specifier for the controller interrupt.
-- clocks : phandle to the Quad SPI clock.
-- cdns,fifo-depth : Size of the data FIFO in words.
-- cdns,fifo-width : Bus width of the data FIFO in bytes.
-- cdns,trigger-address : 32-bit indirect AHB trigger address.
-
-Optional properties:
-- cdns,is-decoded-cs : Flag to indicate whether decoder is used or not.
-- cdns,rclk-en : Flag to indicate that QSPI return clock is used to latch
- the read data rather than the QSPI clock. Make sure that QSPI return
- clock is populated on the board before using this property.
-
-Optional subnodes:
-Subnodes of the Cadence Quad SPI controller are spi slave nodes with additional
-custom properties:
-- cdns,read-delay : Delay for read capture logic, in clock cycles
-- cdns,tshsl-ns : Delay in nanoseconds for the length that the master
- mode chip select outputs are de-asserted between
- transactions.
-- cdns,tsd2d-ns : Delay in nanoseconds between one chip select being
- de-activated and the activation of another.
-- cdns,tchsh-ns : Delay in nanoseconds between last bit of current
- transaction and deasserting the device chip select
- (qspi_n_ss_out).
-- cdns,tslch-ns : Delay in nanoseconds between setting qspi_n_ss_out low
- and first bit transfer.
-- resets : Must contain an entry for each entry in reset-names.
- See ../reset/reset.txt for details.
-- reset-names : Must include either "qspi" and/or "qspi-ocp".
-
-Example:
-
- qspi: spi@ff705000 {
- compatible = "cdns,qspi-nor";
- #address-cells = <1>;
- #size-cells = <0>;
- reg = <0xff705000 0x1000>,
- <0xffa0 0x1000>;
- interrupts = <0 151 4>;
- clocks = <_clk>;
- cdns,is-decoded-cs;
- cdns,fifo-depth = <128>;
- cdns,fifo-width = <4>;
- cdns,trigger-address = <0x>;
- resets = < QSPI_RESET>, < QSPI_OCP_RESET>;
- reset-names = "qspi", "qspi-ocp";
-
- flash0: n25q00@0 {
- ...
- cdns,read-delay = <4>;
- cdns,tshsl-ns = <50>;
- cdns,tsd2d-ns = <50>;
- cdns,tchsh-ns = <4>;
- cdns,tslch-ns = <4>;
- };
- };
diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
new file mode 100644
index ..0e7087cc8bf9
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
@@ -0,0 +1,143 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/cdns,qspi-nor.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Cadence Quad SPI controller
+
+maintainers:
+ - Pratyush Yadav
+
+allOf:
+ - $ref: spi-con
[PATCH 3/4] arm64: dts: ti: k3-am64-main: Fix ospi compatible
The TI specific compatible should be followed by the generic
"cdns,qspi-nor" compatible.
Signed-off-by: Pratyush Yadav
---
arch/arm64/boot/dts/ti/k3-am64-main.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/ti/k3-am64-main.dtsi
b/arch/arm64/boot/dts/ti/k3-am64-main.dtsi
index b997d13f9ec5..3cbdd33384a0 100644
--- a/arch/arm64/boot/dts/ti/k3-am64-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am64-main.dtsi
@@ -592,7 +592,7 @@ fss: bus@fc0 {
ranges;
ospi0: spi@fc4 {
- compatible = "ti,am654-ospi";
+ compatible = "ti,am654-ospi", "cdns,qspi-nor";
reg = <0x00 0x0fc4 0x00 0x100>,
<0x05 0x 0x01 0x>;
interrupts = ;
--
2.30.0
[PATCH 0/4] Convert Cadence QSPI bindings to yaml
Hi, This series picks up Ramuthevar's effort on converting the Cadence QSPI bindings to yaml [0]. During the conversion process, I discovered that some TI device tree files were not using the compatible correctly. Those are fixed in patches 1-3. [0] https://patchwork.kernel.org/project/spi-devel-general/patch/20201116031003.19062-6-vadivel.muruganx.ramuthe...@linux.intel.com/ Pratyush Yadav (3): arm64: dts: ti: k3-j721e-mcu: Fix ospi compatible arm64: dts: ti: k3-j7200-mcu: Fix ospi compatible arm64: dts: ti: k3-am64-main: Fix ospi compatible Ramuthevar Vadivel Murugan (1): dt-bindings: spi: Convert cadence-quadspi.txt to cadence-quadspi.yaml .../bindings/spi/cadence-quadspi.txt | 68 - .../bindings/spi/cdns,qspi-nor.yaml | 143 ++ arch/arm64/boot/dts/ti/k3-am64-main.dtsi | 2 +- .../boot/dts/ti/k3-j7200-mcu-wakeup.dtsi | 2 +- .../boot/dts/ti/k3-j721e-mcu-wakeup.dtsi | 4 +- 5 files changed, 147 insertions(+), 72 deletions(-) delete mode 100644 Documentation/devicetree/bindings/spi/cadence-quadspi.txt create mode 100644 Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml -- 2.30.0
[PATCH 2/4] arm64: dts: ti: k3-j7200-mcu: Fix ospi compatible
The TI specific compatible should be followed by the generic
"cdns,qspi-nor" compatible.
Signed-off-by: Pratyush Yadav
---
arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi
b/arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi
index 5408ec815d58..2ab5a7a15bd5 100644
--- a/arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi
@@ -271,7 +271,7 @@ hbmc: hyperbus@47034000 {
};
ospi0: spi@4704 {
- compatible = "ti,am654-ospi";
+ compatible = "ti,am654-ospi", "cdns,qspi-nor";
reg = <0x0 0x4704 0x0 0x100>,
<0x5 0x 0x1 0x000>;
interrupts = ;
--
2.30.0
[PATCH 1/4] arm64: dts: ti: k3-j721e-mcu: Fix ospi compatible
The TI specific compatible should be followed by the generic
"cdns,qspi-nor" compatible.
Signed-off-by: Pratyush Yadav
---
arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi
b/arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi
index 6c44afae9187..d56e3475aee7 100644
--- a/arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi
@@ -180,7 +180,7 @@ fss: fss@4700 {
ranges;
ospi0: spi@4704 {
- compatible = "ti,am654-ospi";
+ compatible = "ti,am654-ospi", "cdns,qspi-nor";
reg = <0x0 0x4704 0x0 0x100>,
<0x5 0x 0x1 0x000>;
interrupts = ;
@@ -197,7 +197,7 @@ ospi0: spi@4704 {
};
ospi1: spi@4705 {
- compatible = "ti,am654-ospi";
+ compatible = "ti,am654-ospi", "cdns,qspi-nor";
reg = <0x0 0x4705 0x0 0x100>,
<0x7 0x 0x1 0x>;
interrupts = ;
--
2.30.0
Re: [RFC PATCH 1/6] spi: spi-mem: Tell controller when device is ready for calibration
On 24/03/21 12:07AM, Michael Walle wrote:
> Hi Pratyush,
>
> Am 2021-03-11 20:12, schrieb Pratyush Yadav:
> > Some controllers like the Cadence OSPI controller need to perform a
> > calibration sequence to operate at high clock speeds. This calibration
> > should happen after the flash is fully initialized otherwise the
> > calibration might happen in a different SPI mode from the one the flash
> > is finally set to. Add a hook that can be used to tell the controller
> > when the flash is ready for calibration. Whether calibration is needed
> > depends on the controller.
> >
> > Signed-off-by: Pratyush Yadav
> > ---
> > drivers/spi/spi-mem.c | 12
> > include/linux/spi/spi-mem.h | 8
> > 2 files changed, 20 insertions(+)
> >
> > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> > index dc713b0c3c4d..e2f05ad3f4dc 100644
> > --- a/drivers/spi/spi-mem.c
> > +++ b/drivers/spi/spi-mem.c
> > @@ -464,6 +464,18 @@ int spi_mem_adjust_op_size(struct spi_mem *mem,
> > struct spi_mem_op *op)
> > }
> > EXPORT_SYMBOL_GPL(spi_mem_adjust_op_size);
> >
> > +int spi_mem_do_calibration(struct spi_mem *mem, struct spi_mem_op *op)
> > +{
> > + struct spi_controller *ctlr = mem->spi->controller;
> > +
> > + if (!ctlr->mem_ops || !ctlr->mem_ops->do_calibration)
> > + return -EOPNOTSUPP;
> > +
> > + ctlr->mem_ops->do_calibration(mem, op);
>
> Can't a calibration fail?
It can. If it does, the controller falls back to lower speed transfers.
There is not much the upper layer can do about this. That's why it is
not informed whether it succeeded or not.
>
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(spi_mem_do_calibration);
> > +
> > static ssize_t spi_mem_no_dirmap_read(struct spi_mem_dirmap_desc *desc,
> > u64 offs, size_t len, void *buf)
> > {
> > diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
> > index 2b65c9edc34e..97a2d280f2d0 100644
> > --- a/include/linux/spi/spi-mem.h
> > +++ b/include/linux/spi/spi-mem.h
> > @@ -250,6 +250,12 @@ static inline void *spi_mem_get_drvdata(struct
> > spi_mem *mem)
> > * the currently mapped area), and the caller of
> > * spi_mem_dirmap_write() is responsible for calling it again in
> > * this case.
> > + * @do_calibration: perform calibration needed for high SPI clock speed
> > + * operation. Should be called after the SPI memory device has
> > + * been completely initialized. The op passed should contain
> > + * a template for the read operation used for the device so
> > + * the controller can decide what type of calibration is
> > + * required for this type of read.
> > *
> > * This interface should be implemented by SPI controllers providing an
> > * high-level interface to execute SPI memory operation, which is
> > usually the
> > @@ -274,6 +280,7 @@ struct spi_controller_mem_ops {
> >u64 offs, size_t len, void *buf);
> > ssize_t (*dirmap_write)(struct spi_mem_dirmap_desc *desc,
> > u64 offs, size_t len, const void *buf);
> > + void (*do_calibration)(struct spi_mem *mem, struct spi_mem_op *op);
> > };
> >
> > /**
> > @@ -346,6 +353,7 @@ bool spi_mem_dtr_supports_op(struct spi_mem *mem,
> > #endif /* CONFIG_SPI_MEM */
> >
> > int spi_mem_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op);
> > +int spi_mem_do_calibration(struct spi_mem *mem, struct spi_mem_op *op);
> >
> > bool spi_mem_supports_op(struct spi_mem *mem,
> > const struct spi_mem_op *op);
>
> --
> -michael
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [RFC PATCH 2/6] mtd: spi-nor: core: consolidate read op creation
On 24/03/21 12:17AM, Michael Walle wrote:
> Am 2021-03-11 20:12, schrieb Pratyush Yadav:
> > Currently the spi_mem_op to read from the flash is used in two places:
> > spi_nor_create_read_dirmap() and spi_nor_spimem_read_data(). In a later
> > commit this number will increase to three. Instead of repeating the same
> > code thrice, add a function that returns a template of the read op. The
> > callers can then fill in the details like address, data length, data
> > buffer location.
> >
> > Signed-off-by: Pratyush Yadav
> > ---
> > drivers/mtd/spi-nor/core.c | 62 --
> > 1 file changed, 32 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index 4a315cb1c4db..8df009f0 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -183,6 +183,33 @@ static int spi_nor_controller_ops_erase(struct
> > spi_nor *nor, loff_t offs)
> > return nor->controller_ops->erase(nor, offs);
> > }
> >
> > +/**
> > + * spi_nor_spimem_get_read_op() - return a template for the spi_mem_op
> > used for
> > + *reading data from the flash via
> > spi-mem.
> > + * @nor:pointer to 'struct spi_nor'
> > + *
> > + * Return: A template of the 'struct spi_mem_op' for used for reading
> > data from
> > + * the flash. The caller is expected to fill in the address, data
> > length, and
> > + * the data buffer.
> > + */
> > +static struct spi_mem_op spi_nor_spimem_get_read_op(struct spi_nor
> > *nor)
> > +{
> > + struct spi_mem_op op =
> > + SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 0),
> > + SPI_MEM_OP_ADDR(nor->addr_width, 0, 0),
> > + SPI_MEM_OP_DUMMY(nor->read_dummy, 0),
> > + SPI_MEM_OP_DATA_IN(1, NULL, 0));
> > +
> > + spi_nor_spimem_setup_op(nor, , nor->read_proto);
> > +
> > + /* convert the dummy cycles to the number of bytes */
> > + op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
> > + if (spi_nor_protocol_is_dtr(nor->read_proto))
> > + op.dummy.nbytes *= 2;
> > +
> > + return op;
> > +}
> > +
> > /**
> > * spi_nor_spimem_read_data() - read data from flash's memory region
> > via
> > * spi-mem
> > @@ -196,21 +223,14 @@ static int spi_nor_controller_ops_erase(struct
> > spi_nor *nor, loff_t offs)
> > static ssize_t spi_nor_spimem_read_data(struct spi_nor *nor, loff_t
> > from,
> > size_t len, u8 *buf)
> > {
> > - struct spi_mem_op op =
> > - SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 0),
> > - SPI_MEM_OP_ADDR(nor->addr_width, from, 0),
> > - SPI_MEM_OP_DUMMY(nor->read_dummy, 0),
> > - SPI_MEM_OP_DATA_IN(len, buf, 0));
> > + struct spi_mem_op op = spi_nor_spimem_get_read_op(nor);
> > bool usebouncebuf;
> > ssize_t nbytes;
> > int error;
> >
> > - spi_nor_spimem_setup_op(nor, , nor->read_proto);
> > -
> > - /* convert the dummy cycles to the number of bytes */
> > - op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
> > - if (spi_nor_protocol_is_dtr(nor->read_proto))
> > - op.dummy.nbytes *= 2;
> > + op.addr.val = from;
> > + op.data.nbytes = len;
> > + op.data.buf.in = buf;
> >
> > usebouncebuf = spi_nor_spimem_bounce(nor, );
> >
> > @@ -3581,28 +3601,10 @@ EXPORT_SYMBOL_GPL(spi_nor_scan);
> > static int spi_nor_create_read_dirmap(struct spi_nor *nor)
> > {
> > struct spi_mem_dirmap_info info = {
> > - .op_tmpl = SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 0),
> > - SPI_MEM_OP_ADDR(nor->addr_width, 0, 0),
> > - SPI_MEM_OP_DUMMY(nor->read_dummy, 0),
> > - SPI_MEM_OP_DATA_IN(0, NULL, 0)),
> > + .op_tmpl = spi_nor_spimem_get_read_op(nor),
> > .offset = 0,
> > .length = nor->mtd.size,
> > };
> > - struct spi_mem_op *op = _tmpl;
> > -
> > - spi_nor_spimem_setup_op(nor, op, nor->read_proto);
> > -
> > - /* convert the dummy cycles to the number of bytes */
> > - op->dummy.nbytes = (nor->read_dummy * op->dummy.buswidth) / 8;
>
Re: [PATCH v2 2/2] mtd: spi-nor: add initial sysfs support
On 23/03/21 03:31PM, Michael Walle wrote:
> Add support to show the name and JEDEC identifier as well as to dump the
> SFDP table. Not all flashes list their SFDP table contents in their
> datasheet. So having that is useful. It might also be helpful in bug
> reports from users.
Acked-by: Pratyush Yadav
>
> Signed-off-by: Michael Walle
> ---
> drivers/mtd/spi-nor/Makefile | 2 +-
> drivers/mtd/spi-nor/core.c | 5 +++
> drivers/mtd/spi-nor/core.h | 3 ++
> drivers/mtd/spi-nor/sysfs.c | 86
> 4 files changed, 95 insertions(+), 1 deletion(-)
> create mode 100644 drivers/mtd/spi-nor/sysfs.c
>
> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> index 653923896205..aff308f75987 100644
> --- a/drivers/mtd/spi-nor/Makefile
> +++ b/drivers/mtd/spi-nor/Makefile
> @@ -1,6 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
>
> -spi-nor-objs := core.o sfdp.o
> +spi-nor-objs := core.o sfdp.o sysfs.o
> spi-nor-objs += atmel.o
> spi-nor-objs += catalyst.o
> spi-nor-objs += eon.o
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index fbc34158a883..02523ddac612 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3708,6 +3708,10 @@ static int spi_nor_probe(struct spi_mem *spimem)
> if (ret)
> return ret;
>
> + ret = spi_nor_sysfs_create(nor);
> + if (ret)
> + return ret;
> +
> return mtd_device_register(>mtd, data ? data->parts : NULL,
> data ? data->nr_parts : 0);
> }
> @@ -3717,6 +3721,7 @@ static int spi_nor_remove(struct spi_mem *spimem)
> struct spi_nor *nor = spi_mem_get_drvdata(spimem);
>
> spi_nor_restore(nor);
> + spi_nor_sysfs_remove(nor);
>
> /* Clean up MTD stuff. */
> return mtd_device_unregister(>mtd);
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 08d2469837da..599035200a03 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -486,4 +486,7 @@ static struct spi_nor __maybe_unused
> *mtd_to_spi_nor(struct mtd_info *mtd)
> return mtd->priv;
> }
>
> +int spi_nor_sysfs_create(struct spi_nor *nor);
> +void spi_nor_sysfs_remove(struct spi_nor *nor);
> +
> #endif /* __LINUX_MTD_SPI_NOR_INTERNAL_H */
> diff --git a/drivers/mtd/spi-nor/sysfs.c b/drivers/mtd/spi-nor/sysfs.c
> new file mode 100644
> index ..c62cc4d6bce6
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/sysfs.c
> @@ -0,0 +1,86 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#include "core.h"
> +
> +static ssize_t name_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct spi_device *spi = to_spi_device(dev);
> + struct spi_mem *spimem = spi_get_drvdata(spi);
> + struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +
> + return sysfs_emit(buf, "%s\n", nor->info->name);
> +}
> +static DEVICE_ATTR_RO(name);
> +
> +static ssize_t jedec_id_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct spi_device *spi = to_spi_device(dev);
> + struct spi_mem *spimem = spi_get_drvdata(spi);
> + struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +
> + return sysfs_emit(buf, "%*phN\n", nor->info->id_len, nor->info->id);
> +}
> +static DEVICE_ATTR_RO(jedec_id);
> +
> +static struct attribute *spi_nor_sysfs_entries[] = {
> + _attr_name.attr,
> + _attr_jedec_id.attr,
> + NULL
> +};
> +
> +static ssize_t sfdp_read(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *bin_attr, char *buf,
> + loff_t off, size_t count)
> +{
> + struct spi_device *spi = to_spi_device(kobj_to_dev(kobj));
> + struct spi_mem *spimem = spi_get_drvdata(spi);
> + struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> + struct sfdp *sfdp = nor->sfdp;
> + size_t sfdp_size = sfdp->num_dwords * sizeof(*sfdp->dwords);
> +
> + return memory_read_from_buffer(buf, count, , nor->sfdp->dwords,
> +sfdp_size);
> +}
> +static BIN_ATTR_RO(sfdp, 0);
> +
> +static struct bin_attribute *spi_nor_sysfs_bin_entries[] = {
> + _attr_sfdp,
> + NULL
> +};
> +
> +static umode_t spi_nor_sysfs_is_bin_visible(struct kobject *kobj,
> +
Re: [PATCH v2 1/2] mtd: spi-nor: sfdp: save a copy of the SFDP data
On 23/03/21 03:31PM, Michael Walle wrote: > Due to possible mode switching to 8D-8D-8D, it might not be possible to > read the SFDP after the initial probe. To be able to dump the SFDP via > sysfs afterwards, make a complete copy of it. > > Signed-off-by: Michael Walle Reviewed-by: Pratyush Yadav -- Regards, Pratyush Yadav Texas Instruments Inc.
Re: [PATCH 1/2] mtd: spi-nor: sfdp: save a copy of the SFDP data
On 22/03/21 11:31PM, Michael Walle wrote: > Am 2021-03-22 19:42, schrieb Pratyush Yadav: > > On 22/03/21 04:32PM, Michael Walle wrote: > > > Am 2021-03-22 15:21, schrieb Pratyush Yadav: > > > > On 18/03/21 10:24AM, Michael Walle wrote: > > > > > + > > > > > + sfdp->num_dwords = DIV_ROUND_UP(sfdp_size, > > > > > sizeof(*sfdp->dwords)); > > > > > > > > The SFDP spec says that Parameter Table Pointer should be DWORD aligned > > > > and Parameter Table length is specified in number of DWORDs. So, > > > > sfdp_size should always be a multiple of 4. Any SFDP table where this is > > > > not true is an invalid one. > > > > > > > > Also, the spec says "Device behavior when the Read SFDP command crosses > > > > the SFDP structure boundary is not defined". > > > > > > > > So I think this should be a check for alignment instead of a round-up. > > > > > > Well, that woundn't help for debugging. I.e. you also want the SFDP > > > data > > > in cases like this. IMHO we should try hard enough to actually get a > > > reasonable dump. > > > > > > OTOH we also rely on the header and the pointers in the header. Any > > > other ideas, but just to chicken out? > > > > Honestly, I don't think reading past the SFDP boundary would be too bad. > > It probably will just be some garbage data. But if you want to avoid > > that, you can always round down instead of up. > > Like I said, while the storage will be rounded up to a multiple of > DWORDs, only sfdp_size is transferred. Thus it case a pointer is not > DWORD aligned, we end up with zeros at the end. > > I'll add a comment. Right. > > This way you will only > > miss the last DWORD at most. In either case, a warning should be printed > > so this problem can be brought to the user's attention. > > I was about to add a warning/debug message. But its the wrong place. > It should really be checked in the for loop which iterates over the > headers before parsing them. You could check sfdp_size but then two > unaligned param pointers might cancel each other out. > > This can be a seperate patch, besides adding a warning, should there > be any other things to do, e.g. stop parsing and error out? Just removing the bad table from the "tables to parse" list should be the most conservative option. > > .. > > > > > > + goto exit; > > > > > + } > > > > > + > > > > > + err = spi_nor_read_sfdp_dma_unsafe(nor, 0, sfdp_size, > > > > > sfdp->dwords); > > Btw, this can be spi_nor_read_sfdp(). But I'm not sure, what this > whole dma capable buffer should be. Is kmalloc(GFP_KERNEL) > considered DMA safe? I think spi_nor_read_sfdp_dma_unsafe() is meant for buffers that are allocated on stack. Both its current users pass in buffers on the stack. Also see bfa4133795e5 (mtd: spi-nor: fix DMA unsafe buffer issue in spi_nor_read_sfdp(), 2017-09-06). sfdp->dwords is a DMA safe buffer, so you should directly use spi_nor_read_sfdp() here. All spi_nor_read_sfdp_dma_unsafe() does is allocate a buffer using kmalloc(GFP_KERNEL), pass it to spi_nor_read_sfdp() and copy the contents back. > > The buffer ends in spi_nor_read_data(), which is also called from > mtdcore: > > spi_nor_read_sfdp() > spi_nor_read_raw() > spi_nor_read_data() > > mtd_read() > mtd_read_oob() > mtd_read_oob_std() > spi_nor_read() > spi_nor_read_data() > > Is the buffer passed from mtd_read() also DMA-safe? Doesn't the SPI > drivers allocate DMA safe buffers if they need them? SPI MEM at least requires the buffers to be DMA-safe. The comment for data.buf.in says "input buffer (must be DMA-able)". Dunno if mtd_read() always passes a DMA-safe buffer though. -- Regards, Pratyush Yadav Texas Instruments Inc.
Re: [PATCH 1/2] mtd: spi-nor: sfdp: save a copy of the SFDP data
On 22/03/21 04:32PM, Michael Walle wrote:
> Am 2021-03-22 15:21, schrieb Pratyush Yadav:
> > On 18/03/21 10:24AM, Michael Walle wrote:
[...]
> > > @@ -1311,6 +1317,49 @@ int spi_nor_parse_sfdp(struct spi_nor *nor,
> > > }
> > > }
> > >
> > > + /*
> > > + * Cache the complete SFDP data. It is not (easily) possible to
> > > fetch
> > > + * SFDP after probe time and we need it for the sysfs access.
> > > + */
> > > + for (i = 0; i < header.nph; i++) {
> > > + param_header = _headers[i];
> > > + sfdp_size = max_t(size_t, sfdp_size,
> > > + SFDP_PARAM_HEADER_PTP(param_header) +
> > > + SFDP_PARAM_HEADER_PARAM_LEN(param_header));
> >
> > *sigh* If BFPT header was not made a part of the main SFDP header, two
> > "sfdp_size = ..." would not be needed, and we could do it all in one
> > place.
> >
> > You can do that refactor if you're feeling like it, but I won't insist
> > on it.
>
> I think that could be refactored when we also use the SFDP cache for
> the remaining parsing.
Ok.
[...]
> >
> > > +
> > > + sfdp->num_dwords = DIV_ROUND_UP(sfdp_size, sizeof(*sfdp->dwords));
> >
> > The SFDP spec says that Parameter Table Pointer should be DWORD aligned
> > and Parameter Table length is specified in number of DWORDs. So,
> > sfdp_size should always be a multiple of 4. Any SFDP table where this is
> > not true is an invalid one.
> >
> > Also, the spec says "Device behavior when the Read SFDP command crosses
> > the SFDP structure boundary is not defined".
> >
> > So I think this should be a check for alignment instead of a round-up.
>
> Well, that woundn't help for debugging. I.e. you also want the SFDP data
> in cases like this. IMHO we should try hard enough to actually get a
> reasonable dump.
>
> OTOH we also rely on the header and the pointers in the header. Any
> other ideas, but just to chicken out?
Honestly, I don't think reading past the SFDP boundary would be too bad.
It probably will just be some garbage data. But if you want to avoid
that, you can always round down instead of up. This way you will only
miss the last DWORD at most. In either case, a warning should be printed
so this problem can be brought to the user's attention.
>
> > > + sfdp->dwords = devm_kcalloc(dev, sfdp->num_dwords,
> > > + sizeof(*sfdp->dwords), GFP_KERNEL);
> > > + if (!sfdp->dwords) {
> > > + err = -ENOMEM;
> >
> > Free sfdp here since it won't be used any longer.
>
> I see, spi_nor_parse_sfdp() is optional and won't fail the probe.
> Nice catch.
>
> >
> > > + goto exit;
> > > + }
> > > +
> > > + err = spi_nor_read_sfdp_dma_unsafe(nor, 0, sfdp_size, sfdp->dwords);
> > > + if (err < 0) {
> > > + dev_dbg(dev, "failed to read SFDP data\n");
> > > + goto exit;
> >
> > Free sfdp and sfdp->dwords here since they won't be used any longer.
> >
> > > + }
> > > +
> > > + nor->sfdp = sfdp;
> > > +
> > > /*
> > >* Check other parameter headers to get the latest revision of
> > >* the basic flash parameter table.
> > > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> > > index a0d572855444..55c550208949 100644
> > > --- a/include/linux/mtd/spi-nor.h
> > > +++ b/include/linux/mtd/spi-nor.h
> > > @@ -351,6 +351,7 @@ enum spi_nor_cmd_ext {
> > > struct flash_info;
> > > struct spi_nor_manufacturer;
> > > struct spi_nor_flash_parameter;
> > > +struct sfdp;
> >
> > nor->sfdp is a pointer. This should not be needed.
> >
> > >
> > > /**
> > > * struct spi_nor - Structure for defining the SPI NOR layer
> > > @@ -375,6 +376,7 @@ struct spi_nor_flash_parameter;
> > > * @read_proto: the SPI protocol for read operations
> > > * @write_proto: the SPI protocol for write operations
> > > * @reg_proto: the SPI protocol for read_reg/write_reg/erase
> > > operations
> > > + * @sfdp:the SFDP data of the flash
> > > * @controller_ops: SPI NOR controller driver specific operations.
> > > * @params: [FLASH-SPECIFIC] SPI NOR flash parameters and
> > > settings.
> > > * The structure includes legacy flash
> > > parameters and
> > > @@ -404,6 +406,7 @@ struct spi_nor {
> > > boolsst_write_second;
> > > u32 flags;
> > > enum spi_nor_cmd_extcmd_ext_type;
> > > + struct sfdp *sfdp;
> > >
> > > const struct spi_nor_controller_ops *controller_ops;
> > >
> > > --
> > > 2.20.1
> > >
>
> --
> -michael
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [PATCH 2/2] mtd: spi-nor: add initial sysfs support
be NULL.
> +
> + return memory_read_from_buffer(buf, count, , nor->sfdp->dwords,
> +sfdp_size);
> +}
> +static BIN_ATTR_RO(sfdp, PAGE_SIZE);
> +
> +static struct bin_attribute *spi_nor_sysfs_bin_entries[] = {
> + _attr_sfdp,
> + NULL
> +};
> +
> +static umode_t spi_nor_sysfs_is_bin_visible(struct kobject *kobj,
> + struct bin_attribute *attr, int n)
> +{
> + struct spi_device *spi = to_spi_device(kobj_to_dev(kobj));
> + struct spi_mem *spimem = spi_get_drvdata(spi);
> + struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +
> + if (attr == _attr_sfdp && nor->sfdp)
> + return 0444;
> +
> + return 0;
> +}
> +
> +static struct attribute_group spi_nor_sysfs_attr_group = {
> + .name = NULL,
> + .is_bin_visible = spi_nor_sysfs_is_bin_visible,
> + .attrs = spi_nor_sysfs_entries,
> + .bin_attrs = spi_nor_sysfs_bin_entries,
> +};
> +
> +int spi_nor_sysfs_create(struct spi_nor *nor)
> +{
> + return sysfs_create_group(>dev->kobj, _nor_sysfs_attr_group);
> +}
> +
> +void spi_nor_sysfs_remove(struct spi_nor *nor)
> +{
> + sysfs_remove_group(>dev->kobj, _nor_sysfs_attr_group);
> +}
> --
> 2.20.1
>
Rest of it looks good to me.
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [PATCH 1/2] mtd: spi-nor: sfdp: save a copy of the SFDP data
sfdp->dwords), GFP_KERNEL);
> + if (!sfdp->dwords) {
> + err = -ENOMEM;
Free sfdp here since it won't be used any longer.
> + goto exit;
> + }
> +
> + err = spi_nor_read_sfdp_dma_unsafe(nor, 0, sfdp_size, sfdp->dwords);
> + if (err < 0) {
> + dev_dbg(dev, "failed to read SFDP data\n");
> + goto exit;
Free sfdp and sfdp->dwords here since they won't be used any longer.
> + }
> +
> + nor->sfdp = sfdp;
> +
> /*
>* Check other parameter headers to get the latest revision of
>* the basic flash parameter table.
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index a0d572855444..55c550208949 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -351,6 +351,7 @@ enum spi_nor_cmd_ext {
> struct flash_info;
> struct spi_nor_manufacturer;
> struct spi_nor_flash_parameter;
> +struct sfdp;
nor->sfdp is a pointer. This should not be needed.
>
> /**
> * struct spi_nor - Structure for defining the SPI NOR layer
> @@ -375,6 +376,7 @@ struct spi_nor_flash_parameter;
> * @read_proto: the SPI protocol for read operations
> * @write_proto: the SPI protocol for write operations
> * @reg_proto: the SPI protocol for read_reg/write_reg/erase
> operations
> + * @sfdp:the SFDP data of the flash
> * @controller_ops: SPI NOR controller driver specific operations.
> * @params: [FLASH-SPECIFIC] SPI NOR flash parameters and settings.
> * The structure includes legacy flash parameters and
> @@ -404,6 +406,7 @@ struct spi_nor {
> boolsst_write_second;
> u32 flags;
> enum spi_nor_cmd_extcmd_ext_type;
> + struct sfdp *sfdp;
>
> const struct spi_nor_controller_ops *controller_ops;
>
> --
> 2.20.1
>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [PATCH] dt-bindings: spi: Convert Freescale DSPI to json schema
On 16/03/21 12:00AM, Pratyush Yadav wrote: > +Cc mtd list > > Hi, > > On 15/03/21 05:45PM, Kuldeep Singh wrote: > > Convert the Freescale DSPI binding to DT schema format using json-schema. > > > > Signed-off-by: Kuldeep Singh > > --- > > Hi Rob, > > This patch is checked with following commands with no warnings observed. > > make distclean; make allmodconfig; > > make dt_binding_check > > DT_SCHEMA_FILES=Documentation/devicetree/bindings/spi/fsl,spi-fsl-dspi.yaml; > > make dtbs_check > > DT_SCHEMA_FILES=Documentation/devicetree/bindings/spi/fsl,spi-fsl-dspi.yaml > > When I add the "fsl,spi-cs-sck-delay" property under the flash@0 node in > the example and run dt_binding_check, I see the below error: > > > /home/pratyush/src/linux/Documentation/devicetree/bindings/spi/fsl,spi-fsl-dspi.example.dt.yaml: > flash@0: 'fsl,spi-cs-sck-delay' does not match any of the regexes: > '^partition@', 'pinctrl-[0-9]+' > From schema: > /home/pratyush/src/lin/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml > > I am trying to solve a similar problem for the Cadence QSPI controller > binding and I wonder what the best solution for this is. The obvious one > would be to add these properties to jedec,spi-nor.yaml. I haven't > managed to come up with any other solution to this problem. > > Rob, all, any suggestions on how to best model this? Ping. Any ideas? -- Regards, Pratyush Yadav Texas Instruments Inc.
Re: [PATCH v3 1/2] mtd: spi-nor: Move Software Write Protection logic out of the core
On 22/03/21 09:51AM, Tudor Ambarus wrote: > It makes the core file a bit smaller and provides better separation > between the Software Write Protection features and the core logic. > All the next generic software write protection features (e.g. Individual > Block Protection) will reside in swp.c. > > Signed-off-by: Tudor Ambarus Acked-by: Pratyush Yadav -- Regards, Pratyush Yadav Texas Instruments Inc.
Re: [PATCH v3 2/2] mtd: spi-nor: swp: Improve code around spi_nor_check_lock_status_sr()
On 22/03/21 09:51AM, Tudor Ambarus wrote: > - bool return value for spi_nor_check_lock_status_sr(), gets rid of > the return 1, > - introduce temporary variables for better readability. > > Suggested-by: Joe Perches > Signed-off-by: Tudor Ambarus Reviewed-by: Pratyush Yadav -- Regards, Pratyush Yadav Texas Instruments Inc.
Re: [PATCH v2 4/5] mtd: spi-nor: Move Software Write Protection logic out of the core
On 17/03/21 06:09AM, tudor.amba...@microchip.com wrote: > On 3/15/21 8:23 AM, Vignesh Raghavendra wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > > content is safe > > > > On 3/9/21 12:58 PM, tudor.amba...@microchip.com wrote: > >> On 3/8/21 7:28 PM, Vignesh Raghavendra wrote: > >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know > >>> the content is safe > >>> > >>> On 3/6/21 3:20 PM, Tudor Ambarus wrote: > >>>> It makes the core file a bit smaller and provides better separation > >>>> between the Software Write Protection features and the core logic. > >>>> All the next generic software write protection features (e.g. Individual > >>>> Block Protection) will reside in swp.c. > >>>> > >>>> Signed-off-by: Tudor Ambarus > >>>> --- > >>>> drivers/mtd/spi-nor/Makefile | 2 +- > >>>> drivers/mtd/spi-nor/core.c | 407 +- > >>>> drivers/mtd/spi-nor/core.h | 4 + > >>>> drivers/mtd/spi-nor/swp.c| 419 +++ > >>> > >>> Hmmm, name swp.c does not seem intuitive to me. How about expanding it a > >>> bit: > >>> > >>> soft-wr-protect.c or software-write-protect.c ? > > Having in mind that we have the SWP configs, I think I prefer swp.c. > But let's see what majority thinks, we'll do as majority prefers. > Michael, Pratyush? I don't have much of an opinion on this tbh. But I usually prefer short names so I'd go with swp.c here. Maybe also add a comment at the top of the file mentioning the full name "Software Write Protection logic" or something similar for clarification. > > >>> > >> > > cut > > > > > I am not a fan of renaming Kconfig options as it breaks make > > olddefconfig flow which many developers rely on. > > > > I'm fine keeping them as they are for now. If someone else screams we will > reconsider. -- Regards, Pratyush Yadav Texas Instruments Inc.
Re: [PATCH] dt-bindings: spi: Convert Freescale DSPI to json schema
On 16/03/21 06:45PM, Michael Walle wrote: > Am 2021-03-15 19:30, schrieb Pratyush Yadav: > > .. > > > +patternProperties: > > > + "@[0-9a-f]+": > > Shouldn't this be "^.*@[0-9a-f]+$"? The pattern has to match _anywhere_ in the string so both should match the flash node. Your pattern is more "strict" or "precise". See the note at [0]. [0] https://json-schema.org/understanding-json-schema/reference/string.html#regular-expressions > > > > +type: object > > > + > > > +properties: > > > + fsl,spi-cs-sck-delay: > > > +description: > > > + Delay in nanoseconds between activating chip select and > > > the start of > > > + clock signal, at the start of a transfer. > > > +$ref: /schemas/types.yaml#/definitions/uint32 > > > + > > > + fsl,spi-sck-cs-delay: > > > +description: > > > + Delay in nanoseconds between stopping the clock signal and > > > + deactivating chip select, at the end of a transfer. > > > +$ref: /schemas/types.yaml#/definitions/uint32 > > > + > .. > > -michael -- Regards, Pratyush Yadav Texas Instruments Inc.
Re: [RFC PATCH 2/3] mtd: spi-nor: sfdp: fix spi_nor_read_sfdp()
On 16/03/21 12:15PM, Michael Walle wrote:
> Am 2021-03-16 12:04, schrieb Pratyush Yadav:
> > On 12/03/21 08:05PM, Michael Walle wrote:
> > > If spi_nor_read_sfdp() is used after probe, we have to set read_proto
> > > and the read dirmap.
> > >
> > > Signed-off-by: Michael Walle
> > > ---
> > > drivers/mtd/spi-nor/sfdp.c | 8
> > > 1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> > > index b1814afefc33..47634ec9b899 100644
> > > --- a/drivers/mtd/spi-nor/sfdp.c
> > > +++ b/drivers/mtd/spi-nor/sfdp.c
> > > @@ -179,19 +179,27 @@ static int spi_nor_read_sfdp(struct spi_nor
> > > *nor, u32 addr,
> > >size_t len, void *buf)
> > > {
> > > u8 addr_width, read_opcode, read_dummy;
> > > + struct spi_mem_dirmap_desc *rdesc;
> > > + enum spi_nor_protocol read_proto;
> > > int ret;
> > >
> > > read_opcode = nor->read_opcode;
> > > + read_proto = nor->read_proto;
> > > + rdesc = nor->dirmap.rdesc;
> > > addr_width = nor->addr_width;
> > > read_dummy = nor->read_dummy;
> > >
> > > nor->read_opcode = SPINOR_OP_RDSFDP;
> > > + nor->read_proto = SNOR_PROTO_1_1_1;
> > > + nor->dirmap.rdesc = NULL;
> > > nor->addr_width = 3;
> > > nor->read_dummy = 8;
> >
> > NACK. You can't assume the device is still in 1S-1S-1S mode after probe.
> > For example, the s28hs512t flash is switched to 8D-8D-8D mode by the
> > time the probe finishes so this would be an invalid command. Same for
> > any flash that goes into a stateful mode.
>
> I see.
>
> > And you can't even keep using nor->read_proto to read SFDP because the
> > Read SFDP command might not be supported in all modes. xSPI spec
> > (JESD251) says that the Read SFDP command is optional in 8D-8D-8D mode.
> >
> > I think the best approach for this would be to cache the entire SFDP
> > table at parse time. This obviously comes with a memory overhead but I
> > don't think it would be too big. For example, the sfdp table on
> > s28hs512t is 491 bytes and it has 6 tables. Anyway, if the memory usage
> > is too much of a problem we can put the feature behind a config.
>
> I don't like to have it a config option, because then, if you really
> need it, i.e. some user has an unknown flash, it might not be there.
Right. Then let's hope people don't mind us using up an extra half
kilobyte or so.
>
> The next question would be, should I leave the current parsing code
> as is or should I also change that to use the sftp data cache. I'd
> prefer to leave it as is.
For this series its fine if you leave it as is. But eventually it would
be a good idea to convert all SFDP parsers to use the cache to reduce
the number of SFDP reads and potentially speed up flash initialization a
bit.
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [RFC PATCH 1/3] mtd: spi-nor: sfdp: remember sfdp_size
On 16/03/21 12:01PM, Michael Walle wrote:
> Hi Pratyush,
>
> Am 2021-03-16 11:42, schrieb Pratyush Yadav:
> > On 12/03/21 08:05PM, Michael Walle wrote:
> > > Save the sftp_size in the spi_nor struct so we can use it to dump the
> > > SFDP table without parsing the headers again.
> > >
> > > Signed-off-by: Michael Walle
> > > ---
> > > drivers/mtd/spi-nor/sfdp.c | 12
> > > include/linux/mtd/spi-nor.h | 1 +
> > > 2 files changed, 13 insertions(+)
> > >
> > > diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> > > index 25142ec4737b..b1814afefc33 100644
> > > --- a/drivers/mtd/spi-nor/sfdp.c
> > > +++ b/drivers/mtd/spi-nor/sfdp.c
> > > @@ -16,6 +16,7 @@
> > > (((p)->parameter_table_pointer[2] << 16) | \
> > >((p)->parameter_table_pointer[1] << 8) | \
> > >((p)->parameter_table_pointer[0] << 0))
> > > +#define SFDP_PARAM_HEADER_PARAM_LEN(p) ((p)->length * 4)
> > >
> > > #define SFDP_BFPT_ID 0xff00 /* Basic Flash Parameter Table
> > > */
> > > #define SFDP_SECTOR_MAP_ID 0xff81 /* Sector Map Table */
> > > @@ -1263,6 +1264,7 @@ int spi_nor_parse_sfdp(struct spi_nor *nor,
> > > struct sfdp_parameter_header *param_headers = NULL;
> > > struct sfdp_header header;
> > > struct device *dev = nor->dev;
> > > + size_t param_max_offset;
> > > size_t psize;
> > > int i, err;
> > >
> > > @@ -1285,6 +1287,9 @@ int spi_nor_parse_sfdp(struct spi_nor *nor,
> > > bfpt_header->major != SFDP_JESD216_MAJOR)
> > > return -EINVAL;
> > >
> > > + nor->sfdp_size = SFDP_PARAM_HEADER_PTP(bfpt_header) +
> > > + SFDP_PARAM_HEADER_PARAM_LEN(bfpt_header);
> > > +
> > > /*
> > >* Allocate memory then read all parameter headers with a single
> > >* Read SFDP command. These parameter headers will actually be
> > > parsed
> > > @@ -1311,6 +1316,13 @@ int spi_nor_parse_sfdp(struct spi_nor *nor,
> > > }
> > > }
> > >
> > > + for (i = 0; i < header.nph; i++) {
> > > + param_header = _headers[i];
> > > + param_max_offset = SFDP_PARAM_HEADER_PTP(param_header) +
> > > +SFDP_PARAM_HEADER_PARAM_LEN(param_header);
> > > + nor->sfdp_size = max(nor->sfdp_size, param_max_offset);
> > > + }
> > > +
> >
> > I don't see any mention in the SFDP spec (JESD216D-01) that parameter
> > tables have to be contiguous.
>
> ch. 6.1, fig.10 looks like all the headers come after the initial SFDP
> header. If that wasn't the case, how would you find the headers then?
>
> Also ch. 6.3
> """All subsequent parameter headers need to be contiguous and may be
> specified..."""
>
> > In fact, it says that "Parameter tables
> > may be located anywhere in the SFDP space. They do not need to
> > immediately follow the parameter headers".
>
> The tables itself, yes, but not the headers for the tables.
Yes, I was talking about the tables and not the headers. There might be
holes in the SFDP space but I don't think it would be a problem.
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [RFC PATCH 2/3] mtd: spi-nor: sfdp: fix spi_nor_read_sfdp()
On 12/03/21 08:05PM, Michael Walle wrote:
> If spi_nor_read_sfdp() is used after probe, we have to set read_proto
> and the read dirmap.
>
> Signed-off-by: Michael Walle
> ---
> drivers/mtd/spi-nor/sfdp.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index b1814afefc33..47634ec9b899 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -179,19 +179,27 @@ static int spi_nor_read_sfdp(struct spi_nor *nor, u32
> addr,
>size_t len, void *buf)
> {
> u8 addr_width, read_opcode, read_dummy;
> + struct spi_mem_dirmap_desc *rdesc;
> + enum spi_nor_protocol read_proto;
> int ret;
>
> read_opcode = nor->read_opcode;
> + read_proto = nor->read_proto;
> + rdesc = nor->dirmap.rdesc;
> addr_width = nor->addr_width;
> read_dummy = nor->read_dummy;
>
> nor->read_opcode = SPINOR_OP_RDSFDP;
> + nor->read_proto = SNOR_PROTO_1_1_1;
> + nor->dirmap.rdesc = NULL;
> nor->addr_width = 3;
> nor->read_dummy = 8;
NACK. You can't assume the device is still in 1S-1S-1S mode after probe.
For example, the s28hs512t flash is switched to 8D-8D-8D mode by the
time the probe finishes so this would be an invalid command. Same for
any flash that goes into a stateful mode.
And you can't even keep using nor->read_proto to read SFDP because the
Read SFDP command might not be supported in all modes. xSPI spec
(JESD251) says that the Read SFDP command is optional in 8D-8D-8D mode.
I think the best approach for this would be to cache the entire SFDP
table at parse time. This obviously comes with a memory overhead but I
don't think it would be too big. For example, the sfdp table on
s28hs512t is 491 bytes and it has 6 tables. Anyway, if the memory usage
is too much of a problem we can put the feature behind a config.
>
> ret = spi_nor_read_raw(nor, addr, len, buf);
>
> nor->read_opcode = read_opcode;
> + nor->read_proto = read_proto;
> + nor->dirmap.rdesc = rdesc;
> nor->addr_width = addr_width;
> nor->read_dummy = read_dummy;
>
> --
> 2.20.1
>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [RFC PATCH 1/3] mtd: spi-nor: sfdp: remember sfdp_size
On 12/03/21 08:05PM, Michael Walle wrote:
> Save the sftp_size in the spi_nor struct so we can use it to dump the
> SFDP table without parsing the headers again.
>
> Signed-off-by: Michael Walle
> ---
> drivers/mtd/spi-nor/sfdp.c | 12
> include/linux/mtd/spi-nor.h | 1 +
> 2 files changed, 13 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index 25142ec4737b..b1814afefc33 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -16,6 +16,7 @@
> (((p)->parameter_table_pointer[2] << 16) | \
>((p)->parameter_table_pointer[1] << 8) | \
>((p)->parameter_table_pointer[0] << 0))
> +#define SFDP_PARAM_HEADER_PARAM_LEN(p) ((p)->length * 4)
>
> #define SFDP_BFPT_ID 0xff00 /* Basic Flash Parameter Table */
> #define SFDP_SECTOR_MAP_ID 0xff81 /* Sector Map Table */
> @@ -1263,6 +1264,7 @@ int spi_nor_parse_sfdp(struct spi_nor *nor,
> struct sfdp_parameter_header *param_headers = NULL;
> struct sfdp_header header;
> struct device *dev = nor->dev;
> + size_t param_max_offset;
> size_t psize;
> int i, err;
>
> @@ -1285,6 +1287,9 @@ int spi_nor_parse_sfdp(struct spi_nor *nor,
> bfpt_header->major != SFDP_JESD216_MAJOR)
> return -EINVAL;
>
> + nor->sfdp_size = SFDP_PARAM_HEADER_PTP(bfpt_header) +
> + SFDP_PARAM_HEADER_PARAM_LEN(bfpt_header);
> +
> /*
>* Allocate memory then read all parameter headers with a single
>* Read SFDP command. These parameter headers will actually be parsed
> @@ -1311,6 +1316,13 @@ int spi_nor_parse_sfdp(struct spi_nor *nor,
> }
> }
>
> + for (i = 0; i < header.nph; i++) {
> + param_header = _headers[i];
> + param_max_offset = SFDP_PARAM_HEADER_PTP(param_header) +
> +SFDP_PARAM_HEADER_PARAM_LEN(param_header);
> + nor->sfdp_size = max(nor->sfdp_size, param_max_offset);
> + }
> +
I don't see any mention in the SFDP spec (JESD216D-01) that parameter
tables have to be contiguous. In fact, it says that "Parameter tables
may be located anywhere in the SFDP space. They do not need to
immediately follow the parameter headers". But I guess we can just say
the sysfs entry exports the entire SFDP space instead of just the tables
so that is OK.
This patch looks good to me other than the small nitpick that we can
merge this loop and the one below that tries to find the latest BFPT
version.
> /*
>* Check other parameter headers to get the latest revision of
>* the basic flash parameter table.
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index a0d572855444..a58118b8b002 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -404,6 +404,7 @@ struct spi_nor {
> boolsst_write_second;
> u32 flags;
> enum spi_nor_cmd_extcmd_ext_type;
> + size_t sfdp_size;
Documentation for this variable missing.
>
> const struct spi_nor_controller_ops *controller_ops;
>
> --
> 2.20.1
>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [PATCH] dt-bindings: spi: Convert Freescale DSPI to json schema
#address-cells = <2>;
> +#size-cells = <2>;
> +
> +spi@210 {
> +compatible = "fsl,ls1028a-dspi", "fsl,ls1021a-v1.0-dspi";
> +#address-cells = <1>;
> +#size-cells = <0>;
> +reg = <0x0 0x210 0x0 0x1>;
> +interrupts = ;
> +clock-names = "dspi";
> +clocks = < QORIQ_CLK_PLATFORM_PLL QORIQ_CLK_PLL_DIV(2)>;
> +dmas = < 0 62>, < 0 60>;
> +dma-names = "tx", "rx";
> +spi-num-chipselects = <4>;
> +little-endian;
> +
> +flash@0 {
> +compatible = "jedec,spi-nor";
> +spi-max-frequency = <1000>;
> +reg = <0>;
> +};
> +};
> +};
> diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt
> b/Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt
> deleted file mode 100644
> index 30a79da9c039..
> --- a/Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt
> +++ /dev/null
> @@ -1,65 +0,0 @@
> -ARM Freescale DSPI controller
> -
> -Required properties:
> -- compatible : must be one of:
> - "fsl,vf610-dspi",
> - "fsl,ls1021a-v1.0-dspi",
> - "fsl,ls1012a-dspi" (optionally followed by "fsl,ls1021a-v1.0-dspi"),
> - "fsl,ls1028a-dspi",
> - "fsl,ls1043a-dspi" (optionally followed by "fsl,ls1021a-v1.0-dspi"),
> - "fsl,ls1046a-dspi" (optionally followed by "fsl,ls1021a-v1.0-dspi"),
> - "fsl,ls1088a-dspi" (optionally followed by "fsl,ls1021a-v1.0-dspi"),
> - "fsl,ls2080a-dspi" (optionally followed by "fsl,ls2085a-dspi"),
> - "fsl,ls2085a-dspi",
> - "fsl,lx2160a-dspi",
> -- reg : Offset and length of the register set for the device
> -- interrupts : Should contain SPI controller interrupt
> -- clocks: from common clock binding: handle to dspi clock.
> -- clock-names: from common clock binding: Shall be "dspi".
> -- pinctrl-0: pin control group to be used for this controller.
> -- pinctrl-names: must contain a "default" entry.
> -- spi-num-chipselects : the number of the chipselect signals.
> -
> -Optional property:
> -- big-endian: If present the dspi device's registers are implemented
> - in big endian mode.
> -- bus-num : the slave chip chipselect signal number.
> -
> -Optional SPI slave node properties:
> -- fsl,spi-cs-sck-delay: a delay in nanoseconds between activating chip
> - select and the start of clock signal, at the start of a transfer.
> -- fsl,spi-sck-cs-delay: a delay in nanoseconds between stopping the clock
> - signal and deactivating chip select, at the end of a transfer.
> -
> -Example:
> -
> -dspi0@4002c000 {
> - #address-cells = <1>;
> - #size-cells = <0>;
> - compatible = "fsl,vf610-dspi";
> - reg = <0x4002c000 0x1000>;
> - interrupts = <0 67 0x04>;
> - clocks = < VF610_CLK_DSPI0>;
> - clock-names = "dspi";
> - spi-num-chipselects = <5>;
> - bus-num = <0>;
> - pinctrl-names = "default";
> - pinctrl-0 = <_dspi0_1>;
> - big-endian;
> -
> - sflash: at26df081a@0 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> - compatible = "atmel,at26df081a";
> - spi-max-frequency = <1600>;
> - spi-cpol;
> - spi-cpha;
> - reg = <0>;
> - linux,modalias = "m25p80";
> - modal = "at26df081a";
> - fsl,spi-cs-sck-delay = <100>;
> - fsl,spi-sck-cs-delay = <50>;
> - };
> -};
> -
> -
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d92f85ca831d..e2c5b7367db9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7060,7 +7060,7 @@ FREESCALE DSPI DRIVER
> M: Vladimir Oltean
> L: linux-...@vger.kernel.org
> S: Maintained
> -F: Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt
> +F: Documentation/devicetree/bindings/spi/fsl,spi-fsl-dspi.yaml
> F: drivers/spi/spi-fsl-dspi.c
> F: include/linux/spi/spi-fsl-dspi.h
>
> --
> 2.17.1
>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [PATCH] mtd: spi-nor: Update comment about the default flash parameters
On 15/03/21 07:56AM, Tudor Ambarus wrote: > s/legacy/default. spi_nor_info_init_params initializes some default > flash parameters and settings that can be overwritten when parsing > SFDP, or by fixup hooks. There's nothing legacy about them, they are > just some default settings, if not otherwise discovered or specified. > > Signed-off-by: Tudor Ambarus Reviewed-by: Pratyush Yadav > --- > drivers/mtd/spi-nor/core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 4a315cb1c4db..26f681cd3a98 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -2913,7 +2913,7 @@ static void spi_nor_info_init_params(struct spi_nor > *nor) > struct device_node *np = spi_nor_get_flash_node(nor); > u8 i, erase_mask; > > - /* Initialize legacy flash parameters and settings. */ > + /* Initialize default flash parameters and settings. */ > params->quad_enable = spi_nor_sr2_bit1_quad_enable; > params->set_4byte_addr_mode = spansion_set_4byte_addr_mode; > params->setup = spi_nor_default_setup; > -- > 2.25.1 > -- Regards, Pratyush Yadav Texas Instruments Inc.
Re: [PATCH v2 4/5] mtd: spi-nor: Move Software Write Protection logic out of the core
On 15/03/21 06:09AM, tudor.amba...@microchip.com wrote:
> On 3/6/21 1:19 PM, Michael Walle wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> >
> > Am 2021-03-06 10:50, schrieb Tudor Ambarus:
> >> It makes the core file a bit smaller and provides better separation
> >> between the Software Write Protection features and the core logic.
> >> All the next generic software write protection features (e.g.
> >> Individual
> >> Block Protection) will reside in swp.c.
> >>
> >> Signed-off-by: Tudor Ambarus
> >> ---
> >
> > [..]
> >
> >> @@ -3554,6 +3152,9 @@ int spi_nor_scan(struct spi_nor *nor, const char
> >> *name,
> >> if (ret)
> >> return ret;
> >>
> >> + if (nor->params->locking_ops)
> >
> > Should this be in spi_nor_register_locking_ops(), too? I.e.
> >
> > void spi_nor_register_locking_ops() {
> > if (!nor->params->locking_ops)
> > return;
> > ..
> > }
>
> Yes, the checking should be done inside spi_nor_register_locking_ops,
> will move it.
>
> Btw, what do you find a better name, spi_nor_register_locking_ops or
> spi_nor_init_locking_ops? Applies to OTP as well.
On a quick glance, spi_nor_register_locking_ops() can be mistaken to
mean "Register locking ops". That is, ops to lock/unlock flash
registers. If you do want to keep using "register", IMO
spi_nor_locking_ops_register() would be better.
>
> Thanks,
> ta
>
> >
> > I don't have a strong opinion on that so far. I just noticed because
> > I put the check into spi_nor_otp_init() for my OTP series. They should
> > be the same though.
> >
> >> + spi_nor_register_locking_ops(nor);
> >
> > -michael
>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [RFC PATCH 0/6] spi: Add OSPI PHY calibration support for spi-cadence-quadspi
On 12/03/21 11:23AM, tudor.amba...@microchip.com wrote: > On 3/12/21 12:10 PM, Pratyush Yadav wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > > content is safe > > > > On 12/03/21 09:09AM, tudor.amba...@microchip.com wrote: > >> On 3/11/21 9:12 PM, Pratyush Yadav wrote: > >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know > >>> the content is safe > >>> > >>> Hi, > >>> > >>> This series adds support for OSPI PHY calibration on the Cadence OSPI > >>> controller. This calibration procedure is needed to allow high clock > >>> speeds in 8D-8D-8D mode. The procedure reads some pre-determined pattern > >>> data from the flash and runs a sequence of test reads to find out the > >>> optimal delays for high speed transfer. More details on the calibration > >>> procedure in patch 5/6. > >> > >> Can the calibration sequence be avoided if the controller is informed > >> about the frequency on which the flash operates? > > s/frequency/maximum supported frequency by the flash Again, the max frequency does not matter. If it is fast enough PHY calibration is needed to make sure lines are sampled at the correct time. > > > > > Maybe I don't understand this correctly, but there should not be any > > frequency on which the flash operates. The controller drives the SPI > > clock so the frequency is decided by the controller. Sure, there is a > > max supported frequency for the flash but the controller can run it > > slower than that if it wishes. The flash has no say in that. > > > > Anyway, the exact frequency at which the flash is running is not it is > > looking for. More details below. > > I thought about choosing at the controller side: > min(max_frequency_controller, max_frequency_flash) > > And there is also the need of changing the frequency on which an op > runs, like the READ SFDP cmd, for which it is recommended to be run at > 50 MHz, but maybe this is another topic, let's see. Right. This is not directly related to the need for having the calibration. Right now calibration is run only after the flash is fully initialized, so there should not be any SFDP commands from that point on. But if we do want to be conservative about it, a field can be added in spi_mem_op that mentions the maximum speed for the op so the controller can decide accordingly. > > > > >> > >> Can you add more details about the optimal delays? Are we talking about > >> flash's AC characteristics? Is the calibration still needed if the upper > >> layer informs the QSPI controller about the needed delays? > > > > There is usually a delay from when the flash drives the data line (IOW, > > puts a data bit on it) and when the signal reaches the controller. This > > delay can vary by the flash, board, silicon characteristics, > > temperature, etc. > > I wonder whether the delay advertised by the flash matters the most, while > all the other are negligible. The delay advertised by the flash does matter. Specifically, the clock to data output delay. But the IO delay in the host (the delay from the pin to the internal FIFO) matters equally as much. Both are accounted for by the tuning. > When I talk about delay, I'm thinking for example at the delay required > between two consecutive transfers without removing the chip select, or about > the minimum delay needed between the activation or the deactivation of the > chip select. These are all described by the flash. Does your controller have > such fields in its registers, to set such delays? If yes, is the calibration > sequence still needed if all the delays are set correctly? The CS related delays are indeed accounted for by a register in the controller. But this is not the delay the calibration is concerned with. The delays the calibration is concerned with are the clock edge to data transition delay (TX delay), and the DS edge to data transition delay (RX delay). The two are totally unrelated. > > When I hear about "board delays", I think about the impedance of the lines, > which should correspond to the impedance of the Flash's IOs (which depends on > the frequency on which the flash runs). A mechanism to choose the best > frequency and impedance level can be added. Board delays in this case are caused by the length of the wires/lines. Even if the lines are perfectly impedance matched to the flash's IOs, there will be a small time delay from when the data signal is launched by the flash and when it is received by the device. This causes a small but noticeable difference in the timing and consequent
Re: [RFC PATCH 0/6] spi: Add OSPI PHY calibration support for spi-cadence-quadspi
On 12/03/21 02:32PM, Michael Walle wrote: > Am 2021-03-11 20:12, schrieb Pratyush Yadav: > > The main problem here is telling the controller where to find the > > pattern and how to read it. This RFC uses nvmem cells which point to a > > fixed partition containing the data to do the reads. It depends on [0] > > and [1]. > > > > The obvious problem with this is it won't work when the partitions are > > defined via command line. I don't see any good way to add nvmem cells to > > command line partitions. I would like some help or ideas here. We don't > > necessarily have to use nvmem either. Any way that can cleanly and > > consistently let the controller find out where the pattern is stored is > > good. > > The NXP LS1028A SoC has a similar calibration (although there its done > in hardware it seems) and there the datasheet mentions there are flash > devices which supports a preamble before a read function. The preamble > is then some kind of learning pattern. Did you see a flash which actually > supports that in the wild? I can't find any publicly available datasheets > of 8bit I/O SPI NOR flashes. I haven't seen any such flash but it looks like Tudor has. > > -michael -- Regards, Pratyush Yadav Texas Instruments Inc.
Re: [RFC PATCH 0/6] spi: Add OSPI PHY calibration support for spi-cadence-quadspi
On 12/03/21 11:20AM, Michael Walle wrote: > Am 2021-03-12 11:10, schrieb Pratyush Yadav: > > There is usually a delay from when the flash drives the data line (IOW, > > puts a data bit on it) and when the signal reaches the controller. This > > delay can vary by the flash, board, silicon characteristics, > > temperature, etc. > > Temperature might change over time, but the calibration is only done > once. I don't know how much influence the temperature actually has, but > our boards are usually operating from -40°C to +85°C. So there might be > a possible temperature difference of 125K between actual calibration and > when the flash is accessed. The algorithm supports a temperature range of -45 C to +130 C. The temperature is checked at calibration time and adjustments are made to make sure the reads work over the entire temperature range [0]. [0] The current implementation does not have a way to query the temperature from the sensor (see cqspi_get_temp()), so it always assumes the temperature at calibration time is 45 C. But that can be added later once the temperature sensor driver is implemented. -- Regards, Pratyush Yadav Texas Instruments Inc.
Re: [RFC PATCH 4/6] spi: cadence-qspi: Use PHY for DAC reads if possible
On 12/03/21 09:13AM, tudor.amba...@microchip.com wrote: > On 3/11/21 9:12 PM, Pratyush Yadav wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > > content is safe > > > > Check if a read is eligible for PHY and if it is, enable PHY and DQS. > > DQS as in data strobe? Shouldn't the upper layer inform the QSPI controller > whether DS is required or not? Yes, DQS as in data strobe. I need to check this again, but IIRC the controller cannot run in PHY mode unless DS is used. Ideally the upper layer should indeed inform the controller whether DS is supported/in-use or not. That can be used to decide whether PHY mode (and consequently the DS line) is to be used or not. Currently there are only two flashes that use 8D-8D-8D mode (S28HS512T and MT35XU512ABA), and both of them drive the DS line. > > > > > Since PHY reads only work at an address that is 16-byte aligned and of > > size that is a multiple of 16 bytes, read the starting and ending > > unaligned portions without PHY, and only enable PHY for the middle part. > > > > Signed-off-by: Pratyush Yadav > > --- > > drivers/spi/spi-cadence-quadspi.c | 203 ++++++---- > > 1 file changed, 182 insertions(+), 21 deletions(-) > > -- Regards, Pratyush Yadav Texas Instruments Inc.
Re: [RFC PATCH 0/6] spi: Add OSPI PHY calibration support for spi-cadence-quadspi
On 12/03/21 09:09AM, tudor.amba...@microchip.com wrote: > On 3/11/21 9:12 PM, Pratyush Yadav wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > > content is safe > > > > Hi, > > > > This series adds support for OSPI PHY calibration on the Cadence OSPI > > controller. This calibration procedure is needed to allow high clock > > speeds in 8D-8D-8D mode. The procedure reads some pre-determined pattern > > data from the flash and runs a sequence of test reads to find out the > > optimal delays for high speed transfer. More details on the calibration > > procedure in patch 5/6. > > Can the calibration sequence be avoided if the controller is informed > about the frequency on which the flash operates? Maybe I don't understand this correctly, but there should not be any frequency on which the flash operates. The controller drives the SPI clock so the frequency is decided by the controller. Sure, there is a max supported frequency for the flash but the controller can run it slower than that if it wishes. The flash has no say in that. Anyway, the exact frequency at which the flash is running is not it is looking for. More details below. > > Can you add more details about the optimal delays? Are we talking about > flash's AC characteristics? Is the calibration still needed if the upper > layer informs the QSPI controller about the needed delays? There is usually a delay from when the flash drives the data line (IOW, puts a data bit on it) and when the signal reaches the controller. This delay can vary by the flash, board, silicon characteristics, temperature, etc. At lower speeds (25 MHz for example) this delay is not a problem because the clock period is longer so there is much more time to sample the data line. It is very likely the controller will sample at a time when the data line is valid. At high speeds (166 MHz for example), especially in DDR mode, this delay starts to play a larger role because the time to sample the data line is much smaller. Now unless the delay is accounted for, it is possible that the controller samples the data line too late or too early and sees invalid data. These delays depend on physical characteristics so it is not possible for any upper layer to inform the controller about it. How will they even know what the required delay is? In summary, no, there is no way an upper layer can inform the controller about this delay. > > Cheers, > ta > > > > > The main problem here is telling the controller where to find the > > pattern and how to read it. This RFC uses nvmem cells which point to a > > fixed partition containing the data to do the reads. It depends on [0] > > and [1]. > > > > The obvious problem with this is it won't work when the partitions are > > defined via command line. I don't see any good way to add nvmem cells to > > command line partitions. I would like some help or ideas here. We don't > > necessarily have to use nvmem either. Any way that can cleanly and > > consistently let the controller find out where the pattern is stored is > > good. > > > > The dts patch depends on [2]. > > > > Tested on TI's J721E EVM. > > > > [0] > > https://patchwork.ozlabs.org/project/linux-mtd/patch/20210302190012.1255-1-zaj...@gmail.com/ > > [1] > > https://patchwork.ozlabs.org/project/linux-mtd/patch/20210308011853.19360-1-ansuels...@gmail.com/ > > [2] > > https://patchwork.kernel.org/project/linux-arm-kernel/patch/20210305153926.3479-2-p.ya...@ti.com/ > > > > Pratyush Yadav (6): > > spi: spi-mem: Tell controller when device is ready for calibration > > mtd: spi-nor: core: consolidate read op creation > > mtd: spi-nor: core: run calibration when initialization is done > > spi: cadence-qspi: Use PHY for DAC reads if possible > > spi: cadence-qspi: Tune PHY to allow running at higher frequencies > > arm64: dts: ti: k3-j721e-som-p0: Enable PHY calibration > > > > arch/arm64/boot/dts/ti/k3-j721e-som-p0.dtsi | 55 ++ > > drivers/mtd/spi-nor/core.c | 74 +- > > drivers/spi/spi-cadence-quadspi.c | 820 +++- > > drivers/spi/spi-mem.c | 12 + > > include/linux/spi/spi-mem.h | 8 + > > 5 files changed, 916 insertions(+), 53 deletions(-) > > > > -- > > 2.30.0 > > > > > > ___ > > linux-arm-kernel mailing list > > linux-arm-ker...@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > -- Regards, Pratyush Yadav Texas Instruments Inc.
[RFC PATCH 5/6] spi: cadence-qspi: Tune PHY to allow running at higher frequencies
The controller can only run at 1/8 ref clock speed without PHY. With
PHY, it can run at the ref clock speed. So, to enable higher speed
operations, perform the PHY tuning algorithm and determine the RX, TX,
and read delay values for optimal performance. The details of the tuning
algorithm can be found at [0].
To allow this tuning to happen, pre-determined data must be programmed
to the flash at some location. This location is then advertised via a
nvmem cell. Without this data being available, the tuning would fail.
The tuning algorithm is a multi-variable search. The RX and TX delays
need to be found, along with the read delay that would work across a
temperature range. To do that, first the upper and lower RX values at
which the tuning pattern is readable are looked for. This is called the
passing region. The search is performed with Tx = 16 incrementing the
read delay with each iteration. If the two RX values have the same read
delay, the same search is performed with TX = 48.
Once the RX boundaries are found, the TX boundaries are searched for in
a similar fashion with RX set to 1/4 of the RX window (the difference
between the highest and lowest values). And similarly, if the TX
boundaries have the same read delay, the same search is performed with
RX set to 3/4 of the RX window.
There is a region around the boundary of the two passing regions. It is
called the failing region. PHY reads will not work in this region so the
PHY should be tuned as far from it as possible to allow for temperature
variations. This region is found using binary search where the window is
progressively narrowed down until it arrives at the final boundary's
lower and upper limits.
Once PHY is successfully tuned, mark it as usable to allow eligible
operations to run at high speeds. PHY can only be used with DAC mode
reads, and only in chunks of 16 bytes. For all other operations, PHY
mode should be turned off.
[0] https://www.ti.com/lit/pdf/spract2/
Signed-off-by: Pratyush Yadav
---
drivers/spi/spi-cadence-quadspi.c | 617 ++
1 file changed, 617 insertions(+)
diff --git a/drivers/spi/spi-cadence-quadspi.c
b/drivers/spi/spi-cadence-quadspi.c
index e64d8e125263..d304148a4722 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
#define CQSPI_NAME "cadence-qspi"
#define CQSPI_MAX_CHIPSELECT 16
@@ -62,6 +63,9 @@ struct cqspi_flash_pdata {
u8 cs;
booluse_phy;
struct phy_setting phy_setting;
+ struct spi_mem_op phy_read_op;
+ u32 phy_tx_start;
+ u32 phy_tx_end;
};
struct cqspi_st {
@@ -237,6 +241,13 @@ struct cqspi_driver_platdata {
#define CQSPI_REG_POLLING_STATUS 0xB0
#define CQSPI_REG_POLLING_STATUS_DUMMY_LSB 16
+#define CQSPI_REG_PHY_CONFIG 0xB4
+#define CQSPI_REG_PHY_CONFIG_RX_DEL_LSB0
+#define CQSPI_REG_PHY_CONFIG_RX_DEL_MASK 0x7F
+#define CQSPI_REG_PHY_CONFIG_TX_DEL_LSB16
+#define CQSPI_REG_PHY_CONFIG_TX_DEL_MASK 0x7F
+#define CQSPI_REG_PHY_CONFIG_RESYNCBIT(31)
+
#define CQSPI_REG_OP_EXT_LOWER 0xE0
#define CQSPI_REG_OP_EXT_READ_LSB 24
#define CQSPI_REG_OP_EXT_WRITE_LSB 16
@@ -262,6 +273,570 @@ struct cqspi_driver_platdata {
#define CQSPI_IRQ_STATUS_MASK 0x1
+#define CQSPI_PHY_INIT_RD 1
+#define CQSPI_PHY_MAX_RD 4
+#define CQSPI_PHY_MAX_RX 63
+#define CQSPI_PHY_MAX_TX 63
+#define CQSPI_PHY_LOW_RX_BOUND 15
+#define CQSPI_PHY_HIGH_RX_BOUND25
+#define CQSPI_PHY_LOW_TX_BOUND 32
+#define CQSPI_PHY_HIGH_TX_BOUND48
+#define CQSPI_PHY_TX_LOOKUP_LOW_BOUND 24
+#define CQSPI_PHY_TX_LOOKUP_HIGH_BOUND 38
+
+#define CQSPI_PHY_DEFAULT_TEMP 45
+#define CQSPI_PHY_MIN_TEMP -45
+#define CQSPI_PHY_MAX_TEMP 130
+#define CQSPI_PHY_MID_TEMP (CQSPI_PHY_MIN_TEMP + \
+((CQSPI_PHY_MAX_TEMP -
CQSPI_PHY_MIN_TEMP) / 2))
+
+static const u8 phy_tuning_pattern[] = {
+0xFE, 0xFF, 0x01, 0x01, 0x01, 0x01, 0x01, 0x00, 0x00, 0xFE, 0xFE, 0x01, 0x01,
+0x01, 0x01, 0x00, 0x00, 0xFE, 0xFE, 0x01, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x00,
+0x00, 0xFE, 0xFE, 0xFF, 0xFF, 0xFF, 0xFF, 0x00, 0x00, 0xFE, 0xFE, 0xFF, 0x01,
+0x01, 0x01, 0x01, 0x01, 0xFE, 0x00, 0xFE, 0xFE, 0x01, 0x01, 0x01, 0x01, 0xFE,
+0x00, 0xFE, 0xFE, 0x01, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFE, 0x00, 0xFE, 0xFE,
+0xFF, 0xFF, 0xFF, 0xFF, 0xFE, 0x00, 0xFE, 0xFE, 0xFF, 0x01, 0x01, 0x01, 0x01,
+0x01, 0x00, 0xFE, 0xFE, 0xFE, 0x01, 0x01, 0x01, 0x01, 0x00, 0xFE, 0xFE, 0xFE,
+0x01, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x00, 0xFE, 0xFE, 0xFE, 0xFF, 0xFF, 0xFF,
+0xFF, 0x00, 0xFE,
[RFC PATCH 6/6] arm64: dts: ti: k3-j721e-som-p0: Enable PHY calibration
For running the flash in 8D-8D-8D mode at 166 MHz (controller ref clock
speed), PHY calibration procedure needs to run to calculate the proper
line delays.
To perform this calibration, the controller needs to know the location
of the pre-determined calibration pattern on the flash. Add a fixed
partition table that contains the calibration partition, along with the
rest of the partitions for the platform. Also add a nvmem cell that
points to the calibration partition.
Signed-off-by: Pratyush Yadav
---
Based on patch
https://patchwork.kernel.org/project/linux-arm-kernel/patch/20210305153926.3479-2-p.ya...@ti.com/
arch/arm64/boot/dts/ti/k3-j721e-som-p0.dtsi | 55 +
1 file changed, 55 insertions(+)
diff --git a/arch/arm64/boot/dts/ti/k3-j721e-som-p0.dtsi
b/arch/arm64/boot/dts/ti/k3-j721e-som-p0.dtsi
index 2fee2906183d..6013ebb45517 100644
--- a/arch/arm64/boot/dts/ti/k3-j721e-som-p0.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-j721e-som-p0.dtsi
@@ -170,6 +170,8 @@ J721E_WKUP_IOPAD(0x002c, PIN_OUTPUT, 0) /* MCU_OSPI0_CSn0 */
{
pinctrl-names = "default";
pinctrl-0 = <_fss0_ospi0_pins_default>;
+ nvmem-cells = <_calibration_data>;
+ nvmem-cell-names = "calibration";
flash@0{
compatible = "jedec,spi-nor";
@@ -184,6 +186,59 @@ flash@0{
cdns,read-delay = <0>;
#address-cells = <1>;
#size-cells = <1>;
+
+ partitions {
+ compatible = "fixed-partitions";
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ partiiton@0 {
+ label = "ospi.tiboot3";
+ reg = <0x0 0x8>;
+ };
+
+ partition@8 {
+ label = "ospi.tispl";
+ reg = <0x8 0x20>;
+ };
+
+ partition@28 {
+ label = "ospi.u-boot";
+ reg = <0x28 0x40>;
+ };
+
+ partition@68 {
+ label = "ospi.env";
+ reg = <0x68 0x2>;
+ };
+
+ partition@6a {
+ label = "ospi.env.backup";
+ reg = <0x6a 0x2>;
+ };
+
+ partition@0x6c {
+ label = "ospi.sysfw";
+ reg = <0x6c 0x10>;
+ };
+
+ partition@80 {
+ label = "ospi.rootfs";
+ reg = <0x80 0x37e>;
+ };
+
+ calibration_partition: partition@3fe {
+ compatible = "nvmem-cells";
+ label = "ospi.phypattern";
+ reg = <0x3fe 0x2>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ ospi_calibration_data: ospi-calibration-cell@0 {
+ reg = <0x0 0x80>;
+ };
+ };
+ };
};
};
--
2.30.0
[RFC PATCH 4/6] spi: cadence-qspi: Use PHY for DAC reads if possible
Check if a read is eligible for PHY and if it is, enable PHY and DQS.
Since PHY reads only work at an address that is 16-byte aligned and of
size that is a multiple of 16 bytes, read the starting and ending
unaligned portions without PHY, and only enable PHY for the middle part.
Signed-off-by: Pratyush Yadav
---
drivers/spi/spi-cadence-quadspi.c | 203 ++
1 file changed, 182 insertions(+), 21 deletions(-)
diff --git a/drivers/spi/spi-cadence-quadspi.c
b/drivers/spi/spi-cadence-quadspi.c
index e2d6ea833423..e64d8e125263 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -41,19 +41,27 @@
struct cqspi_st;
+struct phy_setting {
+ u8 rx;
+ u8 tx;
+ u8 read_delay;
+};
+
struct cqspi_flash_pdata {
- struct cqspi_st *cqspi;
- u32 clk_rate;
- u32 read_delay;
- u32 tshsl_ns;
- u32 tsd2d_ns;
- u32 tchsh_ns;
- u32 tslch_ns;
- u8 inst_width;
- u8 addr_width;
- u8 data_width;
- booldtr;
- u8 cs;
+ struct cqspi_st *cqspi;
+ u32 clk_rate;
+ u32 read_delay;
+ u32 tshsl_ns;
+ u32 tsd2d_ns;
+ u32 tchsh_ns;
+ u32 tslch_ns;
+ u8 inst_width;
+ u8 addr_width;
+ u8 data_width;
+ booldtr;
+ u8 cs;
+ booluse_phy;
+ struct phy_setting phy_setting;
};
struct cqspi_st {
@@ -108,12 +116,14 @@ struct cqspi_driver_platdata {
/* Register map */
#define CQSPI_REG_CONFIG 0x00
#define CQSPI_REG_CONFIG_ENABLE_MASK BIT(0)
+#define CQSPI_REG_CONFIG_PHY_ENBIT(3)
#define CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL BIT(7)
#define CQSPI_REG_CONFIG_DECODE_MASK BIT(9)
#define CQSPI_REG_CONFIG_CHIPSELECT_LSB10
#define CQSPI_REG_CONFIG_DMA_MASK BIT(15)
#define CQSPI_REG_CONFIG_BAUD_LSB 19
#define CQSPI_REG_CONFIG_DTR_PROTO BIT(24)
+#define CQSPI_REG_CONFIG_PHY_PIPELINE BIT(25)
#define CQSPI_REG_CONFIG_DUAL_OPCODE BIT(30)
#define CQSPI_REG_CONFIG_IDLE_LSB 31
#define CQSPI_REG_CONFIG_CHIPSELECT_MASK 0xF
@@ -150,6 +160,7 @@ struct cqspi_driver_platdata {
#define CQSPI_REG_READCAPTURE_BYPASS_LSB 0
#define CQSPI_REG_READCAPTURE_DELAY_LSB1
#define CQSPI_REG_READCAPTURE_DELAY_MASK 0xF
+#define CQSPI_REG_READCAPTURE_DQS_LSB 8
#define CQSPI_REG_SIZE 0x14
#define CQSPI_REG_SIZE_ADDRESS_LSB 0
@@ -999,6 +1010,7 @@ static void cqspi_config_baudrate_div(struct cqspi_st
*cqspi)
static void cqspi_readdata_capture(struct cqspi_st *cqspi,
const bool bypass,
+ const bool dqs,
const unsigned int delay)
{
void __iomem *reg_base = cqspi->iobase;
@@ -1017,6 +1029,11 @@ static void cqspi_readdata_capture(struct cqspi_st
*cqspi,
reg |= (delay & CQSPI_REG_READCAPTURE_DELAY_MASK)
<< CQSPI_REG_READCAPTURE_DELAY_LSB;
+ if (dqs)
+ reg |= (1 << CQSPI_REG_READCAPTURE_DQS_LSB);
+ else
+ reg &= ~(1 << CQSPI_REG_READCAPTURE_DQS_LSB);
+
writel(reg, reg_base + CQSPI_REG_READCAPTURE);
}
@@ -1035,6 +1052,64 @@ static void cqspi_controller_enable(struct cqspi_st
*cqspi, bool enable)
writel(reg, reg_base + CQSPI_REG_CONFIG);
}
+static void cqspi_phy_enable(struct cqspi_flash_pdata *f_pdata, bool enable)
+{
+ struct cqspi_st *cqspi = f_pdata->cqspi;
+ void __iomem *reg_base = cqspi->iobase;
+ u32 reg;
+ u8 dummy;
+
+ if (enable) {
+ cqspi_readdata_capture(cqspi, 1, true,
+ f_pdata->phy_setting.read_delay);
+
+ reg = readl(reg_base + CQSPI_REG_CONFIG);
+ reg |= CQSPI_REG_CONFIG_PHY_EN |
+ CQSPI_REG_CONFIG_PHY_PIPELINE;
+ writel(reg, reg_base + CQSPI_REG_CONFIG);
+
+ /*
+* Reduce dummy cycle by 1. This is a requirement of PHY mode
+* operation for correctly reading the data.
+*/
+ reg = readl(reg_base + CQSPI_REG_RD_INSTR);
+ dummy = (reg >> CQSPI_REG_RD_INSTR_DUMMY_LSB) &
+ CQSPI_REG_RD_INSTR_DUMMY_MASK;
+ dummy--;
+
[RFC PATCH 1/6] spi: spi-mem: Tell controller when device is ready for calibration
Some controllers like the Cadence OSPI controller need to perform a
calibration sequence to operate at high clock speeds. This calibration
should happen after the flash is fully initialized otherwise the
calibration might happen in a different SPI mode from the one the flash
is finally set to. Add a hook that can be used to tell the controller
when the flash is ready for calibration. Whether calibration is needed
depends on the controller.
Signed-off-by: Pratyush Yadav
---
drivers/spi/spi-mem.c | 12
include/linux/spi/spi-mem.h | 8
2 files changed, 20 insertions(+)
diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index dc713b0c3c4d..e2f05ad3f4dc 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -464,6 +464,18 @@ int spi_mem_adjust_op_size(struct spi_mem *mem, struct
spi_mem_op *op)
}
EXPORT_SYMBOL_GPL(spi_mem_adjust_op_size);
+int spi_mem_do_calibration(struct spi_mem *mem, struct spi_mem_op *op)
+{
+ struct spi_controller *ctlr = mem->spi->controller;
+
+ if (!ctlr->mem_ops || !ctlr->mem_ops->do_calibration)
+ return -EOPNOTSUPP;
+
+ ctlr->mem_ops->do_calibration(mem, op);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(spi_mem_do_calibration);
+
static ssize_t spi_mem_no_dirmap_read(struct spi_mem_dirmap_desc *desc,
u64 offs, size_t len, void *buf)
{
diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index 2b65c9edc34e..97a2d280f2d0 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -250,6 +250,12 @@ static inline void *spi_mem_get_drvdata(struct spi_mem
*mem)
* the currently mapped area), and the caller of
* spi_mem_dirmap_write() is responsible for calling it again in
* this case.
+ * @do_calibration: perform calibration needed for high SPI clock speed
+ * operation. Should be called after the SPI memory device has
+ * been completely initialized. The op passed should contain
+ * a template for the read operation used for the device so
+ * the controller can decide what type of calibration is
+ * required for this type of read.
*
* This interface should be implemented by SPI controllers providing an
* high-level interface to execute SPI memory operation, which is usually the
@@ -274,6 +280,7 @@ struct spi_controller_mem_ops {
u64 offs, size_t len, void *buf);
ssize_t (*dirmap_write)(struct spi_mem_dirmap_desc *desc,
u64 offs, size_t len, const void *buf);
+ void (*do_calibration)(struct spi_mem *mem, struct spi_mem_op *op);
};
/**
@@ -346,6 +353,7 @@ bool spi_mem_dtr_supports_op(struct spi_mem *mem,
#endif /* CONFIG_SPI_MEM */
int spi_mem_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op);
+int spi_mem_do_calibration(struct spi_mem *mem, struct spi_mem_op *op);
bool spi_mem_supports_op(struct spi_mem *mem,
const struct spi_mem_op *op);
--
2.30.0
[RFC PATCH 3/6] mtd: spi-nor: core: run calibration when initialization is done
Once the flash is initialized tell the controller it can run
calibration procedures if needed. This can be useful when calibration is
needed to run at higher clock speeds.
Signed-off-by: Pratyush Yadav
---
drivers/mtd/spi-nor/core.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 8df009f0..e0cbcaf1be89 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3650,6 +3650,7 @@ static int spi_nor_probe(struct spi_mem *spimem)
* checking what's really supported using spi_mem_supports_op().
*/
const struct spi_nor_hwcaps hwcaps = { .mask = SNOR_HWCAPS_ALL };
+ struct spi_mem_op op;
char *flash_name;
int ret;
@@ -3709,8 +3710,15 @@ static int spi_nor_probe(struct spi_mem *spimem)
if (ret)
return ret;
- return mtd_device_register(>mtd, data ? data->parts : NULL,
- data ? data->nr_parts : 0);
+ ret = mtd_device_register(>mtd, data ? data->parts : NULL,
+ data ? data->nr_parts : 0);
+ if (ret)
+ return ret;
+
+ op = spi_nor_spimem_get_read_op(nor);
+ spi_mem_do_calibration(nor->spimem, );
+
+ return 0;
}
static int spi_nor_remove(struct spi_mem *spimem)
--
2.30.0
[RFC PATCH 0/6] spi: Add OSPI PHY calibration support for spi-cadence-quadspi
Hi, This series adds support for OSPI PHY calibration on the Cadence OSPI controller. This calibration procedure is needed to allow high clock speeds in 8D-8D-8D mode. The procedure reads some pre-determined pattern data from the flash and runs a sequence of test reads to find out the optimal delays for high speed transfer. More details on the calibration procedure in patch 5/6. The main problem here is telling the controller where to find the pattern and how to read it. This RFC uses nvmem cells which point to a fixed partition containing the data to do the reads. It depends on [0] and [1]. The obvious problem with this is it won't work when the partitions are defined via command line. I don't see any good way to add nvmem cells to command line partitions. I would like some help or ideas here. We don't necessarily have to use nvmem either. Any way that can cleanly and consistently let the controller find out where the pattern is stored is good. The dts patch depends on [2]. Tested on TI's J721E EVM. [0] https://patchwork.ozlabs.org/project/linux-mtd/patch/20210302190012.1255-1-zaj...@gmail.com/ [1] https://patchwork.ozlabs.org/project/linux-mtd/patch/20210308011853.19360-1-ansuels...@gmail.com/ [2] https://patchwork.kernel.org/project/linux-arm-kernel/patch/20210305153926.3479-2-p.ya...@ti.com/ Pratyush Yadav (6): spi: spi-mem: Tell controller when device is ready for calibration mtd: spi-nor: core: consolidate read op creation mtd: spi-nor: core: run calibration when initialization is done spi: cadence-qspi: Use PHY for DAC reads if possible spi: cadence-qspi: Tune PHY to allow running at higher frequencies arm64: dts: ti: k3-j721e-som-p0: Enable PHY calibration arch/arm64/boot/dts/ti/k3-j721e-som-p0.dtsi | 55 ++ drivers/mtd/spi-nor/core.c | 74 +- drivers/spi/spi-cadence-quadspi.c | 820 +++- drivers/spi/spi-mem.c | 12 + include/linux/spi/spi-mem.h | 8 + 5 files changed, 916 insertions(+), 53 deletions(-) -- 2.30.0
[RFC PATCH 2/6] mtd: spi-nor: core: consolidate read op creation
Currently the spi_mem_op to read from the flash is used in two places:
spi_nor_create_read_dirmap() and spi_nor_spimem_read_data(). In a later
commit this number will increase to three. Instead of repeating the same
code thrice, add a function that returns a template of the read op. The
callers can then fill in the details like address, data length, data
buffer location.
Signed-off-by: Pratyush Yadav
---
drivers/mtd/spi-nor/core.c | 62 --
1 file changed, 32 insertions(+), 30 deletions(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 4a315cb1c4db..8df009f0 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -183,6 +183,33 @@ static int spi_nor_controller_ops_erase(struct spi_nor
*nor, loff_t offs)
return nor->controller_ops->erase(nor, offs);
}
+/**
+ * spi_nor_spimem_get_read_op() - return a template for the spi_mem_op used for
+ *reading data from the flash via spi-mem.
+ * @nor:pointer to 'struct spi_nor'
+ *
+ * Return: A template of the 'struct spi_mem_op' for used for reading data from
+ * the flash. The caller is expected to fill in the address, data length, and
+ * the data buffer.
+ */
+static struct spi_mem_op spi_nor_spimem_get_read_op(struct spi_nor *nor)
+{
+ struct spi_mem_op op =
+ SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 0),
+ SPI_MEM_OP_ADDR(nor->addr_width, 0, 0),
+ SPI_MEM_OP_DUMMY(nor->read_dummy, 0),
+ SPI_MEM_OP_DATA_IN(1, NULL, 0));
+
+ spi_nor_spimem_setup_op(nor, , nor->read_proto);
+
+ /* convert the dummy cycles to the number of bytes */
+ op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
+ if (spi_nor_protocol_is_dtr(nor->read_proto))
+ op.dummy.nbytes *= 2;
+
+ return op;
+}
+
/**
* spi_nor_spimem_read_data() - read data from flash's memory region via
* spi-mem
@@ -196,21 +223,14 @@ static int spi_nor_controller_ops_erase(struct spi_nor
*nor, loff_t offs)
static ssize_t spi_nor_spimem_read_data(struct spi_nor *nor, loff_t from,
size_t len, u8 *buf)
{
- struct spi_mem_op op =
- SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 0),
- SPI_MEM_OP_ADDR(nor->addr_width, from, 0),
- SPI_MEM_OP_DUMMY(nor->read_dummy, 0),
- SPI_MEM_OP_DATA_IN(len, buf, 0));
+ struct spi_mem_op op = spi_nor_spimem_get_read_op(nor);
bool usebouncebuf;
ssize_t nbytes;
int error;
- spi_nor_spimem_setup_op(nor, , nor->read_proto);
-
- /* convert the dummy cycles to the number of bytes */
- op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
- if (spi_nor_protocol_is_dtr(nor->read_proto))
- op.dummy.nbytes *= 2;
+ op.addr.val = from;
+ op.data.nbytes = len;
+ op.data.buf.in = buf;
usebouncebuf = spi_nor_spimem_bounce(nor, );
@@ -3581,28 +3601,10 @@ EXPORT_SYMBOL_GPL(spi_nor_scan);
static int spi_nor_create_read_dirmap(struct spi_nor *nor)
{
struct spi_mem_dirmap_info info = {
- .op_tmpl = SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 0),
- SPI_MEM_OP_ADDR(nor->addr_width, 0, 0),
- SPI_MEM_OP_DUMMY(nor->read_dummy, 0),
- SPI_MEM_OP_DATA_IN(0, NULL, 0)),
+ .op_tmpl = spi_nor_spimem_get_read_op(nor),
.offset = 0,
.length = nor->mtd.size,
};
- struct spi_mem_op *op = _tmpl;
-
- spi_nor_spimem_setup_op(nor, op, nor->read_proto);
-
- /* convert the dummy cycles to the number of bytes */
- op->dummy.nbytes = (nor->read_dummy * op->dummy.buswidth) / 8;
- if (spi_nor_protocol_is_dtr(nor->read_proto))
- op->dummy.nbytes *= 2;
-
- /*
-* Since spi_nor_spimem_setup_op() only sets buswidth when the number
-* of data bytes is non-zero, the data buswidth won't be set here. So,
-* do it explicitly.
-*/
- op->data.buswidth = spi_nor_get_protocol_data_nbits(nor->read_proto);
nor->dirmap.rdesc = devm_spi_mem_dirmap_create(nor->dev, nor->spimem,
);
--
2.30.0
Re: [PATCH v2 3/3] arm64: dts: ti: k3-j7200-som-p0: Add nodes for OSPI0
On 11/03/21 07:22AM, Nishanth Menon wrote:
> On 21:43-20210305, Vignesh Raghavendra wrote:
> >
> >
> > On 3/5/21 9:09 PM, Pratyush Yadav wrote:
> > > TI J7200 has the Cadence OSPI controller for interfacing with OSPI
> > > flashes. Add its node to allow using SPI flashes.
> > >
> > > Signed-off-by: Pratyush Yadav
> > > ---
> >
> > Reviewed-by: Vignesh Raghavendra
> >
> >
> >
> > >
> > > Notes:
> > > Changes in v2:
> > > - Do not force a pulldown on the DQS line because it already has a
> > > pulldown resistor.
> > >
> > > .../boot/dts/ti/k3-j7200-mcu-wakeup.dtsi | 17 +
> > > arch/arm64/boot/dts/ti/k3-j7200-som-p0.dtsi | 36 +++
> > > 2 files changed, 53 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi
> > > b/arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi
> > > index 359e3e8a8cd0..5408ec815d58 100644
> > > --- a/arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi
> > > +++ b/arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi
> > > @@ -269,6 +269,23 @@ hbmc: hyperbus@47034000 {
> > > #size-cells = <1>;
> > > mux-controls = <_mux 0>;
> > > };
> > > +
> > > + ospi0: spi@4704 {
> > > + compatible = "ti,am654-ospi";
> > > + reg = <0x0 0x4704 0x0 0x100>,
> > > + <0x5 0x 0x1 0x000>;
> > > + interrupts = ;
> > > + cdns,fifo-depth = <256>;
> > > + cdns,fifo-width = <4>;
> > > + cdns,trigger-address = <0x0>;
> > > + clocks = <_clks 103 0>;
> > > + assigned-clocks = <_clks 103 0>;
> > > + assigned-clock-parents = <_clks 103 2>;
> > > + assigned-clock-rates = <1>;
> > > + power-domains = <_pds 103 TI_SCI_PD_EXCLUSIVE>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + };
> > > };
> > >
> > > tscadc0: tscadc@4020 {
> > > diff --git a/arch/arm64/boot/dts/ti/k3-j7200-som-p0.dtsi
> > > b/arch/arm64/boot/dts/ti/k3-j7200-som-p0.dtsi
> > > index a988e2ab2ba1..34724440171a 100644
> > > --- a/arch/arm64/boot/dts/ti/k3-j7200-som-p0.dtsi
> > > +++ b/arch/arm64/boot/dts/ti/k3-j7200-som-p0.dtsi
> > > @@ -100,6 +100,22 @@ J721E_WKUP_IOPAD(0x24, PIN_INPUT, 1) /* (A8)
> > > MCU_OSPI0_D6.MCU_HYPERBUS0_DQ6 */
> > > J721E_WKUP_IOPAD(0x28, PIN_INPUT, 1) /* (A7)
> > > MCU_OSPI0_D7.MCU_HYPERBUS0_DQ7 */
> > > >;
> > > };
> > > +
> > > + mcu_fss0_ospi0_pins_default: mcu-fss0-ospi0-pins-default {
> > > + pinctrl-single,pins = <
> > > + J721E_WKUP_IOPAD(0x, PIN_OUTPUT, 0) /*
> > > MCU_OSPI0_CLK */
> > > + J721E_WKUP_IOPAD(0x002c, PIN_OUTPUT, 0) /*
> > > MCU_OSPI0_CSn0 */
> > > + J721E_WKUP_IOPAD(0x000c, PIN_INPUT, 0) /* MCU_OSPI0_D0
> > > */
> > > + J721E_WKUP_IOPAD(0x0010, PIN_INPUT, 0) /* MCU_OSPI0_D1
> > > */
> > > + J721E_WKUP_IOPAD(0x0014, PIN_INPUT, 0) /* MCU_OSPI0_D2
> > > */
> > > + J721E_WKUP_IOPAD(0x0018, PIN_INPUT, 0) /* MCU_OSPI0_D3
> > > */
> > > + J721E_WKUP_IOPAD(0x001c, PIN_INPUT, 0) /* MCU_OSPI0_D4
> > > */
> > > + J721E_WKUP_IOPAD(0x0020, PIN_INPUT, 0) /* MCU_OSPI0_D5
> > > */
> > > + J721E_WKUP_IOPAD(0x0024, PIN_INPUT, 0) /* MCU_OSPI0_D6
> > > */
> > > + J721E_WKUP_IOPAD(0x0028, PIN_INPUT, 0) /* MCU_OSPI0_D7
> > > */
> > > + J721E_WKUP_IOPAD(0x0008, PIN_INPUT, 0) /*
> > > MCU_OSPI0_DQS */
> > > + >;
> > > + };
> > > };
> > >
> > > _pmx0 {
> > > @@ -235,3 +251,23 @@ exp_som: gpio@21 {
> > > "GPIO_LIN_EN", "CAN_STB";
> > > };
> > > };
> > > +
> > > + {
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <_f
Re: [PATCH 2/2] arm64: dts: ti: k3-am64-evm/sk: Add OSPI flash DT node
On 09/03/21 06:35PM, Vignesh Raghavendra wrote:
> Both AM64 EVM and SK have a 512Mb S28HS512T Octal SPI NOR flash.
> Add DT node for the same.
>
> Signed-off-by: Vignesh Raghavendra
Reviewed-by: Pratyush Yadav
> ---
>
> Bootlog:
>
> SK: https://pastebin.ubuntu.com/p/gvxg7cFrXH/
> EVM: https://pastebin.ubuntu.com/p/jb39GqkB78/
>
> arch/arm64/boot/dts/ti/k3-am642-evm.dts | 36 +
> arch/arm64/boot/dts/ti/k3-am642-sk.dts | 36 +
> 2 files changed, 72 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/ti/k3-am642-evm.dts
> b/arch/arm64/boot/dts/ti/k3-am642-evm.dts
> index 1f1787750fef..7dd8e94b108d 100644
> --- a/arch/arm64/boot/dts/ti/k3-am642-evm.dts
> +++ b/arch/arm64/boot/dts/ti/k3-am642-evm.dts
> @@ -133,6 +133,22 @@ AM64X_IOPAD(0x0268, PIN_INPUT_PULLUP, 0) /* (C18)
> I2C1_SCL */
> AM64X_IOPAD(0x026c, PIN_INPUT_PULLUP, 0) /* (B19)
> I2C1_SDA */
> >;
> };
> +
> + ospi0_pins_default: ospi0-pins-default {
> + pinctrl-single,pins = <
> + AM64X_IOPAD(0x, PIN_OUTPUT, 0) /* (N20) OSPI0_CLK */
> + AM64X_IOPAD(0x002c, PIN_OUTPUT, 0) /* (L19) OSPI0_CSn0
> */
> + AM64X_IOPAD(0x000c, PIN_INPUT, 0) /* (M19) OSPI0_D0 */
> + AM64X_IOPAD(0x0010, PIN_INPUT, 0) /* (M18) OSPI0_D1 */
> + AM64X_IOPAD(0x0014, PIN_INPUT, 0) /* (M20) OSPI0_D2 */
> + AM64X_IOPAD(0x0018, PIN_INPUT, 0) /* (M21) OSPI0_D3 */
> + AM64X_IOPAD(0x001c, PIN_INPUT, 0) /* (P21) OSPI0_D4 */
> + AM64X_IOPAD(0x0020, PIN_INPUT, 0) /* (P20) OSPI0_D5 */
> + AM64X_IOPAD(0x0024, PIN_INPUT, 0) /* (N18) OSPI0_D6 */
> + AM64X_IOPAD(0x0028, PIN_INPUT, 0) /* (M17) OSPI0_D7 */
> + AM64X_IOPAD(0x0008, PIN_INPUT, 0) /* (N19) OSPI0_DQS */
> + >;
> + };
> };
>
> _uart0 {
> @@ -244,3 +260,23 @@ {
> ti,driver-strength-ohm = <50>;
> disable-wp;
> };
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_pins_default>;
> +
> + flash@0{
> + compatible = "jedec,spi-nor";
> + reg = <0x0>;
> + spi-tx-bus-width = <8>;
> + spi-rx-bus-width = <8>;
> + spi-max-frequency = <2500>;
> + cdns,tshsl-ns = <60>;
> + cdns,tsd2d-ns = <60>;
> + cdns,tchsh-ns = <60>;
> + cdns,tslch-ns = <60>;
> + cdns,read-delay = <4>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + };
> +};
> diff --git a/arch/arm64/boot/dts/ti/k3-am642-sk.dts
> b/arch/arm64/boot/dts/ti/k3-am642-sk.dts
> index aa6ca4c49153..fc27470fc083 100644
> --- a/arch/arm64/boot/dts/ti/k3-am642-sk.dts
> +++ b/arch/arm64/boot/dts/ti/k3-am642-sk.dts
> @@ -90,6 +90,22 @@ AM64X_IOPAD(0x0268, PIN_INPUT_PULLUP, 0) /* (C18) I2C1_SCL
> */
> AM64X_IOPAD(0x026c, PIN_INPUT_PULLUP, 0) /* (B19)
> I2C1_SDA */
> >;
> };
> +
> + ospi0_pins_default: ospi0-pins-default {
> + pinctrl-single,pins = <
> + AM64X_IOPAD(0x, PIN_OUTPUT, 0) /* (N20) OSPI0_CLK */
> + AM64X_IOPAD(0x002c, PIN_OUTPUT, 0) /* (L19) OSPI0_CSn0
> */
> + AM64X_IOPAD(0x000c, PIN_INPUT, 0) /* (M19) OSPI0_D0 */
> + AM64X_IOPAD(0x0010, PIN_INPUT, 0) /* (M18) OSPI0_D1 */
> + AM64X_IOPAD(0x0014, PIN_INPUT, 0) /* (M20) OSPI0_D2 */
> + AM64X_IOPAD(0x0018, PIN_INPUT, 0) /* (M21) OSPI0_D3 */
> + AM64X_IOPAD(0x001c, PIN_INPUT, 0) /* (P21) OSPI0_D4 */
> + AM64X_IOPAD(0x0020, PIN_INPUT, 0) /* (P20) OSPI0_D5 */
> + AM64X_IOPAD(0x0024, PIN_INPUT, 0) /* (N18) OSPI0_D6 */
> + AM64X_IOPAD(0x0028, PIN_INPUT, 0) /* (M17) OSPI0_D7 */
> + AM64X_IOPAD(0x0008, PIN_INPUT, 0) /* (N19) OSPI0_DQS */
> + >;
> + };
> };
>
> _uart0 {
> @@ -171,3 +187,23 @@ {
> ti,driver-strength-ohm = <50>;
> disable-wp;
> };
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_pins_default>;
> +
> + flash@0{
> + compatible = "jedec,spi-nor";
> + reg = <0x0>;
> + spi-tx-bus-width = <8>;
> + spi-rx-bus-width = <8>;
> + spi-max-frequency = <2500>;
> + cdns,tshsl-ns = <60>;
> + cdns,tsd2d-ns = <60>;
> + cdns,tchsh-ns = <60>;
> + cdns,tslch-ns = <60>;
> + cdns,read-delay = <4>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + };
> +};
> --
> 2.30.1
>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [PATCH 1/2] arm64: dts: ti: k3-am64-main: Add OSPI node
On 09/03/21 06:35PM, Vignesh Raghavendra wrote:
> AM64 SoC has a single Octal SPI (OSPI) instance under Flash SubSystem
> (FSS). Add DT entry for the same.
>
> Signed-off-by: Vignesh Raghavendra
Reviewed-by: Pratyush Yadav
> ---
> arch/arm64/boot/dts/ti/k3-am64-main.dtsi | 25
> 1 file changed, 25 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/ti/k3-am64-main.dtsi
> b/arch/arm64/boot/dts/ti/k3-am64-main.dtsi
> index 5f85950daef7..bcec4fa444b5 100644
> --- a/arch/arm64/boot/dts/ti/k3-am64-main.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am64-main.dtsi
> @@ -402,4 +402,29 @@ sdhci1: mmc@fa0 {
> ti,otap-del-sel-ddr50 = <0x9>;
> ti,clkbuf-sel = <0x7>;
> };
> +
> + fss: bus@fc0 {
> + compatible = "simple-bus";
> + reg = <0x00 0x0fc0 0x00 0x7>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> + ranges;
> +
> + ospi0: spi@fc4 {
> + compatible = "ti,am654-ospi", "cdns,qspi-nor";
> + reg = <0x00 0x0fc4 0x00 0x100>,
> + <0x05 0x 0x01 0x>;
> + interrupts = ;
> + cdns,fifo-depth = <256>;
> + cdns,fifo-width = <4>;
> + cdns,trigger-address = <0x0>;
> + #address-cells = <0x1>;
> + #size-cells = <0x0>;
> + clocks = <_clks 75 6>;
> + assigned-clocks = <_clks 75 6>;
> + assigned-clock-parents = <_clks 75 7>;
> + assigned-clock-rates = <1>;
> + power-domains = <_pds 75 TI_SCI_PD_EXCLUSIVE>;
> + };
> + };
> };
> --
> 2.30.1
>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [PATCH] MAINTAINERS: Add Michael and Pratyush as designated reviewers for SPI NOR
On 08/03/21 11:23AM, Tudor Ambarus wrote: > It's already been the case for some time that Michael and Pratyush > are reviewing SPI NOR patches. Update MAINTAINERS to reflect reality. > > Signed-off-by: Tudor Ambarus Acked-by: Pratyush Yadav > --- > Michael, Pratyush, please send your Acked-by tags if you agree. > > MAINTAINERS | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index d92f85ca831d..ba561e5bc6f0 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -16862,6 +16862,8 @@ F:arch/arm/mach-spear/ > > SPI NOR SUBSYSTEM > M: Tudor Ambarus > +R: Michael Walle > +R: Pratyush Yadav > L: linux-...@lists.infradead.org > S: Maintained > W: http://www.linux-mtd.infradead.org/ > -- > 2.25.1 > -- Regards, Pratyush Yadav Texas Instruments Inc.
Re: [PATCH] mtd: spi-nor: use is_power_of_2()
On 06/03/21 12:45AM, Michael Walle wrote:
> There is already a function to check if an integer is a power of 2. Use
> it.
>
> Signed-off-by: Michael Walle
> ---
> drivers/mtd/spi-nor/core.c | 5 +
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 0522304f52fa..4a315cb1c4db 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2336,11 +2336,8 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t
> to, size_t len,
>* If page_size is a power of two, the offset can be quickly
>* calculated with an AND operation. On the other cases we
>* need to do a modulus operation (more expensive).
> - * Power of two numbers have only one bit set and we can use
> - * the instruction hweight32 to detect if we need to do a
> - * modulus (do_div()) or not.
>*/
> - if (hweight32(nor->page_size) == 1) {
> + if (is_power_of_2(nor->page_size)) {
> page_offset = addr & (nor->page_size - 1);
> } else {
> uint64_t aux = addr;
Reviewed-by: Pratyush Yadav
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [PATCH v2 5/5] mtd: spi-nor: swp: Drop 'else' after 'return'
On 06/03/21 11:50AM, Tudor Ambarus wrote: > else is not generally useful after a break or return. > > Signed-off-by: Tudor Ambarus Reviewed-by: Pratyush Yadav -- Regards, Pratyush Yadav Texas Instruments Inc.
Re: [PATCH v2 3/5] mtd: spi-nor: Get rid of duplicated argument in spi_nor_parse_sfdp()
On 06/03/21 11:50AM, Tudor Ambarus wrote:
> spi_nor_parse_sfdp(nor, nor->params);
> passes for the second argument a member within the first argument.
> Drop the second argument and obtain it directly from the first,
> and do it across all the children functions. This is a follow up for
> 'commit 69a8eed58cc0 ("mtd: spi-nor: Don't copy self-pointing struct around")'
>
> Signed-off-by: Tudor Ambarus
Reviewed-by: Pratyush Yadav
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [PATCH v2 2/5] mtd: spi-nor: core: Add vdbg msg for spi_nor_erase_multi_sectors()
On 06/03/21 11:49AM, Tudor Ambarus wrote:
> Useful when debugging non-uniform erase.
>
> Signed-off-by: Tudor Ambarus
> ---
> v2:
> - s/dev_dbg/dev_vdb
> - move vdbg message the first thing in the while
>
> drivers/mtd/spi-nor/core.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index bcaa161bc7db..498da1ec3a89 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1610,6 +1610,9 @@ static int spi_nor_erase_multi_sectors(struct spi_nor
> *nor, u64 addr, u32 len)
> list_for_each_entry_safe(cmd, next, _list, list) {
> nor->erase_opcode = cmd->opcode;
> while (cmd->count) {
> + dev_vdbg(nor->dev, "erase_cmd->size = 0x%08x,
> erase_cmd->opcode = 0x%02x, erase_cmd->count = %d\n",
erase_cmd->count is an unsigned value (u32) so it should be %u instead
of %d.
Other than this,
Reviewed-by: Pratyush Yadav
> + cmd->size, cmd->opcode, cmd->count);
> +
> ret = spi_nor_write_enable(nor);
> if (ret)
> goto destroy_erase_cmd_list;
> --
> 2.25.1
>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
[PATCH v2 3/3] arm64: dts: ti: k3-j7200-som-p0: Add nodes for OSPI0
TI J7200 has the Cadence OSPI controller for interfacing with OSPI
flashes. Add its node to allow using SPI flashes.
Signed-off-by: Pratyush Yadav
---
Notes:
Changes in v2:
- Do not force a pulldown on the DQS line because it already has a
pulldown resistor.
.../boot/dts/ti/k3-j7200-mcu-wakeup.dtsi | 17 +
arch/arm64/boot/dts/ti/k3-j7200-som-p0.dtsi | 36 +++
2 files changed, 53 insertions(+)
diff --git a/arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi
b/arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi
index 359e3e8a8cd0..5408ec815d58 100644
--- a/arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi
@@ -269,6 +269,23 @@ hbmc: hyperbus@47034000 {
#size-cells = <1>;
mux-controls = <_mux 0>;
};
+
+ ospi0: spi@4704 {
+ compatible = "ti,am654-ospi";
+ reg = <0x0 0x4704 0x0 0x100>,
+ <0x5 0x 0x1 0x000>;
+ interrupts = ;
+ cdns,fifo-depth = <256>;
+ cdns,fifo-width = <4>;
+ cdns,trigger-address = <0x0>;
+ clocks = <_clks 103 0>;
+ assigned-clocks = <_clks 103 0>;
+ assigned-clock-parents = <_clks 103 2>;
+ assigned-clock-rates = <1>;
+ power-domains = <_pds 103 TI_SCI_PD_EXCLUSIVE>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ };
};
tscadc0: tscadc@4020 {
diff --git a/arch/arm64/boot/dts/ti/k3-j7200-som-p0.dtsi
b/arch/arm64/boot/dts/ti/k3-j7200-som-p0.dtsi
index a988e2ab2ba1..34724440171a 100644
--- a/arch/arm64/boot/dts/ti/k3-j7200-som-p0.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-j7200-som-p0.dtsi
@@ -100,6 +100,22 @@ J721E_WKUP_IOPAD(0x24, PIN_INPUT, 1) /* (A8)
MCU_OSPI0_D6.MCU_HYPERBUS0_DQ6 */
J721E_WKUP_IOPAD(0x28, PIN_INPUT, 1) /* (A7)
MCU_OSPI0_D7.MCU_HYPERBUS0_DQ7 */
>;
};
+
+ mcu_fss0_ospi0_pins_default: mcu-fss0-ospi0-pins-default {
+ pinctrl-single,pins = <
+ J721E_WKUP_IOPAD(0x, PIN_OUTPUT, 0) /*
MCU_OSPI0_CLK */
+ J721E_WKUP_IOPAD(0x002c, PIN_OUTPUT, 0) /*
MCU_OSPI0_CSn0 */
+ J721E_WKUP_IOPAD(0x000c, PIN_INPUT, 0) /* MCU_OSPI0_D0
*/
+ J721E_WKUP_IOPAD(0x0010, PIN_INPUT, 0) /* MCU_OSPI0_D1
*/
+ J721E_WKUP_IOPAD(0x0014, PIN_INPUT, 0) /* MCU_OSPI0_D2
*/
+ J721E_WKUP_IOPAD(0x0018, PIN_INPUT, 0) /* MCU_OSPI0_D3
*/
+ J721E_WKUP_IOPAD(0x001c, PIN_INPUT, 0) /* MCU_OSPI0_D4
*/
+ J721E_WKUP_IOPAD(0x0020, PIN_INPUT, 0) /* MCU_OSPI0_D5
*/
+ J721E_WKUP_IOPAD(0x0024, PIN_INPUT, 0) /* MCU_OSPI0_D6
*/
+ J721E_WKUP_IOPAD(0x0028, PIN_INPUT, 0) /* MCU_OSPI0_D7
*/
+ J721E_WKUP_IOPAD(0x0008, PIN_INPUT, 0) /*
MCU_OSPI0_DQS */
+ >;
+ };
};
_pmx0 {
@@ -235,3 +251,23 @@ exp_som: gpio@21 {
"GPIO_LIN_EN", "CAN_STB";
};
};
+
+ {
+ pinctrl-names = "default";
+ pinctrl-0 = <_fss0_ospi0_pins_default>;
+
+ flash@0{
+ compatible = "jedec,spi-nor";
+ reg = <0x0>;
+ spi-tx-bus-width = <8>;
+ spi-rx-bus-width = <8>;
+ spi-max-frequency = <2500>;
+ cdns,tshsl-ns = <60>;
+ cdns,tsd2d-ns = <60>;
+ cdns,tchsh-ns = <60>;
+ cdns,tslch-ns = <60>;
+ cdns,read-delay = <4>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ };
+};
--
2.30.0
[PATCH v2 2/3] arm64: dts: ti: am654-base-board: Enable 8D-8D-8D mode on OSPI
Set the Tx bus width to 8 so 8D-8D-8D mode can be selected. Change the
frequency to 25 MHz. This is the frequency that the flash has been
successfully tested with in Octal DTR mode. The total performance should
still increase since 8D-8D-8D mode should be at least twice as fast as
1S-1S-8S mode.
Signed-off-by: Pratyush Yadav
---
Notes:
No changes in v2.
arch/arm64/boot/dts/ti/k3-am654-base-board.dts | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
index fe3043943906..9e87fb313a54 100644
--- a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
+++ b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
@@ -483,9 +483,9 @@ {
flash@0{
compatible = "jedec,spi-nor";
reg = <0x0>;
- spi-tx-bus-width = <1>;
+ spi-tx-bus-width = <8>;
spi-rx-bus-width = <8>;
- spi-max-frequency = <4000>;
+ spi-max-frequency = <2500>;
cdns,tshsl-ns = <60>;
cdns,tsd2d-ns = <60>;
cdns,tchsh-ns = <60>;
--
2.30.0
[PATCH v2 1/3] arm64: dts: ti: k3-j721e-som-p0: Enable 8D-8D-8D mode on OSPI
Set the Tx bus width to 8 so 8D-8D-8D mode can be selected. Change the
frequency to 25 MHz. This is the frequency that the flash has been
successfully tested with in Octal DTR mode. The total performance should
still increase since 8D-8D-8D mode should be at least twice as fast as
1S-1S-8S mode.
Signed-off-by: Pratyush Yadav
---
Notes:
No changes in v2.
arch/arm64/boot/dts/ti/k3-j721e-som-p0.dtsi | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/ti/k3-j721e-som-p0.dtsi
b/arch/arm64/boot/dts/ti/k3-j721e-som-p0.dtsi
index 57720e6a04c5..2fee2906183d 100644
--- a/arch/arm64/boot/dts/ti/k3-j721e-som-p0.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-j721e-som-p0.dtsi
@@ -174,9 +174,9 @@ {
flash@0{
compatible = "jedec,spi-nor";
reg = <0x0>;
- spi-tx-bus-width = <1>;
+ spi-tx-bus-width = <8>;
spi-rx-bus-width = <8>;
- spi-max-frequency = <4000>;
+ spi-max-frequency = <2500>;
cdns,tshsl-ns = <60>;
cdns,tsd2d-ns = <60>;
cdns,tchsh-ns = <60>;
--
2.30.0
[PATCH v2 0/3] Enable 8D-8D-8D mode on J721E, J7200, AM654
Hi, Now that the OSPI controller driver and the SPI NOR core have support for 8D-8D-8D mode, the device tree can be updated to allow Octal DTR transactions. Pratyush Yadav (3): arm64: dts: ti: k3-j721e-som-p0: Enable 8D-8D-8D mode on OSPI arm64: dts: ti: am654-base-board: Enable 8D-8D-8D mode on OSPI arm64: dts: ti: k3-j7200-som-p0: Add nodes for OSPI0 .../arm64/boot/dts/ti/k3-am654-base-board.dts | 4 +-- .../boot/dts/ti/k3-j7200-mcu-wakeup.dtsi | 17 + arch/arm64/boot/dts/ti/k3-j7200-som-p0.dtsi | 36 +++ arch/arm64/boot/dts/ti/k3-j721e-som-p0.dtsi | 4 +-- 4 files changed, 57 insertions(+), 4 deletions(-) -- 2.30.0
Re: [ANNOUNCE] Git v2.31.0-rc1
On 03/03/21 10:14PM, Junio C Hamano wrote:
> Eric Sunshine writes:
>
> > On Wed, Mar 3, 2021 at 7:23 PM Junio C Hamano wrote:
> >> Pratyush Yadav (1):
> >> git-gui: remove lines starting with the comment character
> >
> > Is there some way that this can be removed from v2.31.0 before final
> > release? It badly breaks git-gui on macOS[1,2] to the point of making
> > it unusable (Tcl throws errors at launch time and when trying to
> > commit, and committing is 100% broken).
>
> Thanks.
>
> I could revert the merge with the problematic changes to git-gui,
> i.e. 0917373 (Merge https://github.com/prati0100/git-gui,
> 2021-03-01), but if possible, I'd rather merge a revert made on the
> git-gui side. If b1056f60 (Merge branch 'py/commit-comments',
> 2021-02-22) is the tip of git-gui repository, and b9a43869 (git-gui:
> remove lines starting with the comment character, 2021-02-03) is
> what breaks, perhaps
>
> $ git checkout b1056f60^2 &&
> git revert b9a43869 &&
> git checkout b1056f60 &&
> git merge @{-1}
>
> would be what we want to have at the tip of git-gui until the
> breakage gets sorted out.
>
> Pratyush?
I will send a follow-up PR with the patch reverted. I'll then apply
Eric's patch to fix breakage on MacOS and let it simmer till the next
release. Let's not risk any breaking changes close to release.
--
Regards,
Pratyush Yadav
Re: [PATCH 3/3] arm64: dts: ti: k3-j7200-som-p0: Add nodes for OSPI0
On 02/03/21 01:10PM, Vignesh Raghavendra wrote:
>
>
> On 3/2/21 1:28 AM, Pratyush Yadav wrote:
> > +
> > + mcu_fss0_ospi0_pins_default: mcu-fss0-ospi0-pins-default {
> > + pinctrl-single,pins = <
> > + J721E_WKUP_IOPAD(0x, PIN_OUTPUT, 0) /*
> > MCU_OSPI0_CLK */
> > + J721E_WKUP_IOPAD(0x002c, PIN_OUTPUT, 0) /*
> > MCU_OSPI0_CSn0 */
> > + J721E_WKUP_IOPAD(0x000c, PIN_INPUT, 0) /* MCU_OSPI0_D0
> > */
> > + J721E_WKUP_IOPAD(0x0010, PIN_INPUT, 0) /* MCU_OSPI0_D1
> > */
> > + J721E_WKUP_IOPAD(0x0014, PIN_INPUT, 0) /* MCU_OSPI0_D2
> > */
> > + J721E_WKUP_IOPAD(0x0018, PIN_INPUT, 0) /* MCU_OSPI0_D3
> > */
> > + J721E_WKUP_IOPAD(0x001c, PIN_INPUT, 0) /* MCU_OSPI0_D4
> > */
> > + J721E_WKUP_IOPAD(0x0020, PIN_INPUT, 0) /* MCU_OSPI0_D5
> > */
> > + J721E_WKUP_IOPAD(0x0024, PIN_INPUT, 0) /* MCU_OSPI0_D6
> > */
> > + J721E_WKUP_IOPAD(0x0028, PIN_INPUT, 0) /* MCU_OSPI0_D7
> > */
> > + J721E_WKUP_IOPAD(0x0008, PIN_INPUT_PULLDOWN, 0) /*
> > MCU_OSPI0_DQS */
> > + >;
> > + };
>
> There is a pulldown resistor on the board right? So, internal pulldown
> is unnecessary and may even cause conflicts.
Right. Will fix.
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
[PATCH 2/3] arm64: dts: ti: am654-base-board: Enable 8D-8D-8D mode on OSPI
Set the Tx bus width to 8 so 8D-8D-8D mode can be selected. Change the
frequency to 25 MHz. This is the frequency that the flash has been
successfully tested with in Octal DTR mode. The total performance should
still increase since 8D-8D-8D mode should be at least twice as fast as
1S-1S-8S mode.
Signed-off-by: Pratyush Yadav
---
arch/arm64/boot/dts/ti/k3-am654-base-board.dts | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
index fe3043943906..9e87fb313a54 100644
--- a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
+++ b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
@@ -483,9 +483,9 @@ {
flash@0{
compatible = "jedec,spi-nor";
reg = <0x0>;
- spi-tx-bus-width = <1>;
+ spi-tx-bus-width = <8>;
spi-rx-bus-width = <8>;
- spi-max-frequency = <4000>;
+ spi-max-frequency = <2500>;
cdns,tshsl-ns = <60>;
cdns,tsd2d-ns = <60>;
cdns,tchsh-ns = <60>;
--
2.30.0
[PATCH 1/3] arm64: dts: ti: k3-j721e-som-p0: Enable 8D-8D-8D mode on OSPI
Set the Tx bus width to 8 so 8D-8D-8D mode can be selected. Change the
frequency to 25 MHz. This is the frequency that the flash has been
successfully tested with in Octal DTR mode. The total performance should
still increase since 8D-8D-8D mode should be at least twice as fast as
1S-1S-8S mode.
Signed-off-by: Pratyush Yadav
---
arch/arm64/boot/dts/ti/k3-j721e-som-p0.dtsi | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/ti/k3-j721e-som-p0.dtsi
b/arch/arm64/boot/dts/ti/k3-j721e-som-p0.dtsi
index 57720e6a04c5..2fee2906183d 100644
--- a/arch/arm64/boot/dts/ti/k3-j721e-som-p0.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-j721e-som-p0.dtsi
@@ -174,9 +174,9 @@ {
flash@0{
compatible = "jedec,spi-nor";
reg = <0x0>;
- spi-tx-bus-width = <1>;
+ spi-tx-bus-width = <8>;
spi-rx-bus-width = <8>;
- spi-max-frequency = <4000>;
+ spi-max-frequency = <2500>;
cdns,tshsl-ns = <60>;
cdns,tsd2d-ns = <60>;
cdns,tchsh-ns = <60>;
--
2.30.0
[PATCH 3/3] arm64: dts: ti: k3-j7200-som-p0: Add nodes for OSPI0
TI J7200 has the Cadence OSPI controller for interfacing with OSPI
flashes. Add its node to allow using SPI flashes.
Signed-off-by: Pratyush Yadav
---
.../boot/dts/ti/k3-j7200-mcu-wakeup.dtsi | 17 +
arch/arm64/boot/dts/ti/k3-j7200-som-p0.dtsi | 36 +++
2 files changed, 53 insertions(+)
diff --git a/arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi
b/arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi
index 359e3e8a8cd0..5408ec815d58 100644
--- a/arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi
@@ -269,6 +269,23 @@ hbmc: hyperbus@47034000 {
#size-cells = <1>;
mux-controls = <_mux 0>;
};
+
+ ospi0: spi@4704 {
+ compatible = "ti,am654-ospi";
+ reg = <0x0 0x4704 0x0 0x100>,
+ <0x5 0x 0x1 0x000>;
+ interrupts = ;
+ cdns,fifo-depth = <256>;
+ cdns,fifo-width = <4>;
+ cdns,trigger-address = <0x0>;
+ clocks = <_clks 103 0>;
+ assigned-clocks = <_clks 103 0>;
+ assigned-clock-parents = <_clks 103 2>;
+ assigned-clock-rates = <1>;
+ power-domains = <_pds 103 TI_SCI_PD_EXCLUSIVE>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ };
};
tscadc0: tscadc@4020 {
diff --git a/arch/arm64/boot/dts/ti/k3-j7200-som-p0.dtsi
b/arch/arm64/boot/dts/ti/k3-j7200-som-p0.dtsi
index a988e2ab2ba1..effecf852139 100644
--- a/arch/arm64/boot/dts/ti/k3-j7200-som-p0.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-j7200-som-p0.dtsi
@@ -100,6 +100,22 @@ J721E_WKUP_IOPAD(0x24, PIN_INPUT, 1) /* (A8)
MCU_OSPI0_D6.MCU_HYPERBUS0_DQ6 */
J721E_WKUP_IOPAD(0x28, PIN_INPUT, 1) /* (A7)
MCU_OSPI0_D7.MCU_HYPERBUS0_DQ7 */
>;
};
+
+ mcu_fss0_ospi0_pins_default: mcu-fss0-ospi0-pins-default {
+ pinctrl-single,pins = <
+ J721E_WKUP_IOPAD(0x, PIN_OUTPUT, 0) /*
MCU_OSPI0_CLK */
+ J721E_WKUP_IOPAD(0x002c, PIN_OUTPUT, 0) /*
MCU_OSPI0_CSn0 */
+ J721E_WKUP_IOPAD(0x000c, PIN_INPUT, 0) /* MCU_OSPI0_D0
*/
+ J721E_WKUP_IOPAD(0x0010, PIN_INPUT, 0) /* MCU_OSPI0_D1
*/
+ J721E_WKUP_IOPAD(0x0014, PIN_INPUT, 0) /* MCU_OSPI0_D2
*/
+ J721E_WKUP_IOPAD(0x0018, PIN_INPUT, 0) /* MCU_OSPI0_D3
*/
+ J721E_WKUP_IOPAD(0x001c, PIN_INPUT, 0) /* MCU_OSPI0_D4
*/
+ J721E_WKUP_IOPAD(0x0020, PIN_INPUT, 0) /* MCU_OSPI0_D5
*/
+ J721E_WKUP_IOPAD(0x0024, PIN_INPUT, 0) /* MCU_OSPI0_D6
*/
+ J721E_WKUP_IOPAD(0x0028, PIN_INPUT, 0) /* MCU_OSPI0_D7
*/
+ J721E_WKUP_IOPAD(0x0008, PIN_INPUT_PULLDOWN, 0) /*
MCU_OSPI0_DQS */
+ >;
+ };
};
_pmx0 {
@@ -235,3 +251,23 @@ exp_som: gpio@21 {
"GPIO_LIN_EN", "CAN_STB";
};
};
+
+ {
+ pinctrl-names = "default";
+ pinctrl-0 = <_fss0_ospi0_pins_default>;
+
+ flash@0{
+ compatible = "jedec,spi-nor";
+ reg = <0x0>;
+ spi-tx-bus-width = <8>;
+ spi-rx-bus-width = <8>;
+ spi-max-frequency = <2500>;
+ cdns,tshsl-ns = <60>;
+ cdns,tsd2d-ns = <60>;
+ cdns,tchsh-ns = <60>;
+ cdns,tslch-ns = <60>;
+ cdns,read-delay = <4>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ };
+};
--
2.30.0
[PATCH 0/3] Enable 8D-8D-8D mode on J721E, J7200, AM654
Hi, Now that the OSPI controller driver and the SPI NOR core have support for 8D-8D-8D mode, the device tree can be updated to allow Octal DTR transactions. Pratyush Yadav (3): arm64: dts: ti: k3-j721e-som-p0: Enable 8D-8D-8D mode on OSPI arm64: dts: ti: am654-base-board: Enable 8D-8D-8D mode on OSPI arm64: dts: ti: k3-j7200-som-p0: Add nodes for OSPI0 .../arm64/boot/dts/ti/k3-am654-base-board.dts | 4 +-- .../boot/dts/ti/k3-j7200-mcu-wakeup.dtsi | 17 + arch/arm64/boot/dts/ti/k3-j7200-som-p0.dtsi | 36 +++ arch/arm64/boot/dts/ti/k3-j721e-som-p0.dtsi | 4 +-- 4 files changed, 57 insertions(+), 4 deletions(-) -- 2.30.0
Re: [PATCH] [v2] spi: rockchip: avoid objtool warning
On 26/02/21 03:00PM, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> Building this file with clang leads to a an unreachable code path
> causing a warning from objtool:
>
> drivers/spi/spi-rockchip.o: warning: objtool:
> rockchip_spi_transfer_one()+0x2e0: sibling call from callable instruction
> with modified stack frame
>
> Change the unreachable() into an error return that can be
> handled if it ever happens, rather than silently crashing
> the kernel.
>
> Fixes: 65498c6ae241 ("spi: rockchip: support 4bit words")
> Signed-off-by: Arnd Bergmann
Acked-by: Pratyush Yadav
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [PATCH] spi: rockchip: avoid objtool warning
On 26/02/21 10:49AM, Arnd Bergmann wrote:
> On Fri, Feb 26, 2021 at 9:16 AM 'Pratyush Yadav' via Clang Built Linux
> wrote:
> >
> > Hi,
> >
> > On 25/02/21 01:55PM, Arnd Bergmann wrote:
> > > From: Arnd Bergmann
> > >
> > > Building this file with clang leads to a an unreachable code path
> > > causing a warning from objtool:
> > >
> > > drivers/spi/spi-rockchip.o: warning: objtool:
> > > rockchip_spi_transfer_one()+0x2e0: sibling call from callable instruction
> > > with modified stack frame
> > >
> > > Use BUG() instead of unreachable() to avoid the undefined behavior
> > > if it does happen.
> > >
> > > Fixes: 65498c6ae241 ("spi: rockchip: support 4bit words")
> > > Signed-off-by: Arnd Bergmann
> > > ---
> > > drivers/spi/spi-rockchip.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
> > > index 936ef54e0903..972beac1169a 100644
> > > --- a/drivers/spi/spi-rockchip.c
> > > +++ b/drivers/spi/spi-rockchip.c
> > > @@ -521,7 +521,7 @@ static void rockchip_spi_config(struct rockchip_spi
> > > *rs,
> > >* ctlr->bits_per_word_mask, so this shouldn't
> > >* happen
> > >*/
> > > - unreachable();
> > > + BUG();
> >
> > checkpatch says:
> >
> > Avoid crashing the kernel - try using WARN_ON & recovery code rather
> > than BUG() or BUG_ON()
> >
> > Which makes sense to me. This is not something bad enough to justify
> > crashing the kernel.
>
> I thought about rewriting it more thoroughly when I wrote the patch, but
> couldn't come up with a good alternative, so I did the simplest change
> in this direction, replacing the silent crash with a loud one.
>
> Should we just dev_warn() and return instead, hoping that it won't do
> more harm?
Hmm... I'm not very familiar with this device or driver so take what I
say with a grain of salt. On the surface level it looks like it might
end up doing something wrong or unexpected.
Returning an error code from this function (along with the dev_warn() or
WARN_ON()) is the most sensible thing to do IMO. If the SPI layer sends
an invalid value then the driver should be well within its rights to
refuse the transaction. The function is currently void but making it
return int seems fairly straightforward.
>
> The backtrace from WARN_ON() probably doesn't help here.
>
>Arnd
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [PATCH] spi: rockchip: avoid objtool warning
Hi,
On 25/02/21 01:55PM, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> Building this file with clang leads to a an unreachable code path
> causing a warning from objtool:
>
> drivers/spi/spi-rockchip.o: warning: objtool:
> rockchip_spi_transfer_one()+0x2e0: sibling call from callable instruction
> with modified stack frame
>
> Use BUG() instead of unreachable() to avoid the undefined behavior
> if it does happen.
>
> Fixes: 65498c6ae241 ("spi: rockchip: support 4bit words")
> Signed-off-by: Arnd Bergmann
> ---
> drivers/spi/spi-rockchip.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
> index 936ef54e0903..972beac1169a 100644
> --- a/drivers/spi/spi-rockchip.c
> +++ b/drivers/spi/spi-rockchip.c
> @@ -521,7 +521,7 @@ static void rockchip_spi_config(struct rockchip_spi *rs,
>* ctlr->bits_per_word_mask, so this shouldn't
>* happen
>*/
> - unreachable();
> + BUG();
checkpatch says:
Avoid crashing the kernel - try using WARN_ON & recovery code rather
than BUG() or BUG_ON()
Which makes sense to me. This is not something bad enough to justify
crashing the kernel.
> }
>
> if (use_dma) {
> --
> 2.29.2
>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [PATCH] mtd: spi-nor: sfdp: Fix out of bound array access
On 12/02/21 04:47PM, Mathieu Dubois-Briand wrote:
> Fix array index: explicitly use the array length to access the last
> element, instead of an incorrectly set iteration variable.
>
> It seems this code was correct before following commit, were the
> iteration counter is reused, leading to a value that may be out of
> bound.
> Fixes: dc92843159a7 ("mtd: spi-nor: fix erase_type array to indicate
> current map conf")
>
> Signed-off-by: Mathieu Dubois-Briand
> ---
> drivers/mtd/spi-nor/sfdp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index 6ee7719e5903..11cc5d19e286 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -881,7 +881,7 @@ spi_nor_init_non_uniform_erase_map(struct spi_nor *nor,
> if (!(regions_erase_type & BIT(erase[i].idx)))
> spi_nor_set_erase_type([i], 0, 0xFF);
>
> - spi_nor_region_mark_end([i - 1]);
> + spi_nor_region_mark_end([region_count - 1]);
I'm not too familiar with the non-uniform erase code but this looks good
at first look. Small nitpick: move this line just after the above for
loop that initializes this array.
>
> return 0;
> }
> --
> 2.25.1
>
>
> __
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Re: [PATCH 1/2] mtd: spi-nor: core: Advance erase after the erase cmd has been completed
On 05/02/21 03:52PM, Tudor Ambarus wrote: > Wait for the erase cmd to complete and then advance the erase. > > Signed-off-by: Tudor Ambarus > --- > drivers/mtd/spi-nor/core.c | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 0522304f52fa..bcaa161bc7db 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -1618,12 +1618,12 @@ static int spi_nor_erase_multi_sectors(struct spi_nor > *nor, u64 addr, u32 len) > if (ret) > goto destroy_erase_cmd_list; > > - addr += cmd->size; > - cmd->count--; > - > ret = spi_nor_wait_till_ready(nor); > if (ret) > goto destroy_erase_cmd_list; > + > + addr += cmd->size; > + cmd->count--; > } > list_del(>list); > kfree(cmd); > @@ -1704,12 +1704,12 @@ static int spi_nor_erase(struct mtd_info *mtd, struct > erase_info *instr) > if (ret) > goto erase_err; > > - addr += mtd->erasesize; > - len -= mtd->erasesize; > - > ret = spi_nor_wait_till_ready(nor); > if (ret) > goto erase_err; > + > + addr += mtd->erasesize; > + len -= mtd->erasesize; Do these changes have any practical benefit? IMO they are worth doing even if there is none but I'm curious what prompted this patch. Reviewed-by: Pratyush Yadav > } > > /* erase multiple sectors */ -- Regards, Pratyush Yadav Texas Instruments Inc.
