Re: [PATCH] drm/vc4: hdmi: Add a name to the codec DAI component
On Wed, Oct 28, 2020 at 01:30:32PM +, Dave Stevenson wrote: > Hi Maxime > > On Wed, 8 Jul 2020 at 15:46, Maxime Ripard wrote: > > > > Since the components for a given device in ASoC are identified by their > > name, it makes sense to add one even though it's not strictly necessary. > > > > Signed-off-by: Maxime Ripard > > Reviewed-by: Dave Stevenson Applied, thanks! Maxime signature.asc Description: PGP signature
Re: [PATCH] iommu/sun50i: check return value of of_find_device_by_node() in sun50i_iommu_of_xlate()
On Thu, Oct 29, 2020 at 05:22:44PM +0800, Yu Kuai wrote:
> If of_find_device_by_node() failed in sun50i_iommu_of_xlate(), null
> pointer dereference will be triggered. Thus return error code if
> of_find_device_by_node() failed.
>
> Fixes: 4100b8c229b3("iommu: Add Allwinner H6 IOMMU driver")
> Signed-off-by: Yu Kuai
Acked-by: Maxime Ripard
Thanks!
Maxime
signature.asc
Description: PGP signature
Re: [PATCH] [v3] rtc: sun6i: Fix memleak in sun6i_rtc_clk_init
On Tue, Oct 20, 2020 at 02:12:26PM +0800, Dinghao Liu wrote: > When clk_hw_register_fixed_rate_with_accuracy() fails, > clk_data should be freed. It's the same for the subsequent > two error paths, but we should also unregister the already > registered clocks in them. > > Signed-off-by: Dinghao Liu Acked-by: Maxime Ripard Maxime
Re: [PATCH] thermal: sun8i: Use bitmap API instead of open code
Hi Frank,
On Mon, Oct 19, 2020 at 07:58:36PM +0800, Frank Lee wrote:
> From: Yangtao Li
>
> Thw bitmap_* API is the standard way to access data in the bitfield.
>
> Signed-off-by: Yangtao Li
> ---
> drivers/thermal/sun8i_thermal.c | 35 +
> 1 file changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
> index f8b13071a6f4..f2e4a4f18101 100644
> --- a/drivers/thermal/sun8i_thermal.c
> +++ b/drivers/thermal/sun8i_thermal.c
> @@ -8,6 +8,7 @@
> * Based on the work of Josef Gajdusek
> */
>
> +#include
> #include
> #include
> #include
> @@ -74,7 +75,7 @@ struct ths_thermal_chip {
> int (*calibrate)(struct ths_device *tmdev,
>u16 *caldata, int callen);
> int (*init)(struct ths_device *tmdev);
> - int (*irq_ack)(struct ths_device *tmdev);
> + void(*irq_ack)(struct ths_device *tmdev);
> int (*calc_temp)(struct ths_device *tmdev,
>int id, int reg);
> };
> @@ -82,6 +83,7 @@ struct ths_thermal_chip {
> struct ths_device {
> const struct ths_thermal_chip *chip;
> struct device *dev;
> + DECLARE_BITMAP(irq_bitmap, MAX_SENSOR_NUM);
> struct regmap *regmap;
> struct reset_control*reset;
> struct clk *bus_clk;
> @@ -146,9 +148,11 @@ static const struct regmap_config config = {
> .max_register = 0xfc,
> };
>
> -static int sun8i_h3_irq_ack(struct ths_device *tmdev)
> +static void sun8i_h3_irq_ack(struct ths_device *tmdev)
> {
> - int i, state, ret = 0;
> + int i, state;
> +
> + bitmap_zero(tmdev->irq_bitmap, tmdev->chip->sensor_num);
>
> regmap_read(tmdev->regmap, SUN8I_THS_IS, );
>
> @@ -156,16 +160,16 @@ static int sun8i_h3_irq_ack(struct ths_device *tmdev)
> if (state & SUN8I_THS_DATA_IRQ_STS(i)) {
> regmap_write(tmdev->regmap, SUN8I_THS_IS,
>SUN8I_THS_DATA_IRQ_STS(i));
> - ret |= BIT(i);
> + bitmap_set(tmdev->irq_bitmap, i, 1);
> }
> }
> -
> - return ret;
> }
>
> -static int sun50i_h6_irq_ack(struct ths_device *tmdev)
> +static void sun50i_h6_irq_ack(struct ths_device *tmdev)
> {
> - int i, state, ret = 0;
> + int i, state;
> +
> + bitmap_zero(tmdev->irq_bitmap, tmdev->chip->sensor_num);
>
> regmap_read(tmdev->regmap, SUN50I_H6_THS_DIS, );
>
> @@ -173,24 +177,21 @@ static int sun50i_h6_irq_ack(struct ths_device *tmdev)
> if (state & SUN50I_H6_THS_DATA_IRQ_STS(i)) {
> regmap_write(tmdev->regmap, SUN50I_H6_THS_DIS,
>SUN50I_H6_THS_DATA_IRQ_STS(i));
> - ret |= BIT(i);
> + bitmap_set(tmdev->irq_bitmap, i, 1);
> }
> }
> -
> - return ret;
The bitfield of the acked interrupts is mostly something internal to the
handler, so I'm not really convinced about sharing that through the
global structure.
With that being said...
> }
>
> static irqreturn_t sun8i_irq_thread(int irq, void *data)
> {
> struct ths_device *tmdev = data;
> - int i, state;
> + int i;
>
> - state = tmdev->chip->irq_ack(tmdev);
> + tmdev->chip->irq_ack(tmdev);
>
> - for (i = 0; i < tmdev->chip->sensor_num; i++) {
> - if (state & BIT(i))
> - thermal_zone_device_update(tmdev->sensor[i].tzd,
> -THERMAL_EVENT_UNSPECIFIED);
> + for_each_set_bit(i, tmdev->irq_bitmap, tmdev->chip->sensor_num) {
> + thermal_zone_device_update(tmdev->sensor[i].tzd,
> +THERMAL_EVENT_UNSPECIFIED);
.. for_each_set_bit is definitely cleaner and more readable.
Since it can operate on any unsigned long pointer, maybe we can just
make irq_ack return an unsigned long, and iterate over it here?
Maxime
signature.asc
Description: PGP signature
Re: [PATCH] drm/vc4: hdmi: Avoid sleeping in atomic context
On Tue, Oct 27, 2020 at 11:15:58AM +0100, Maxime Ripard wrote:
> When running the trigger hook, ALSA by default will take a spinlock, and
> thus will run the trigger hook in atomic context.
>
> However, our HDMI driver will send the infoframes as part of the trigger
> hook, and part of that process is to wait for a bit to be cleared for up to
> 100ms. To be nicer to the system, that wait has some usleep_range that
> interact poorly with the atomic context.
>
> There's several ways we can fix this, but the more obvious one is to make
> ALSA take a mutex instead by setting the nonatomic flag on the DAI link.
> That doesn't work though, since now the cyclic callback installed by the
> dmaengine helpers in ALSA will take a mutex, while that callback is run by
> dmaengine's virt-chan code in a tasklet where sleeping is not allowed
> either.
>
> Given the delay we need to poll the bit for, changing the usleep_range for
> a udelay and keep running it from a context where interrupts are disabled
> is not really a good option either.
>
> However, we can move the infoframe setup code in the hw_params hook, like
> is usually done in other HDMI controllers, that isn't protected by a
> spinlock and thus where we can sleep. Infoframes will be sent on a regular
> basis anyway, and since hw_params is where the audio parameters that end up
> in the infoframes are setup, this also makes a bit more sense.
>
> Fixes: bb7d78568814 ("drm/vc4: Add HDMI audio support")
> Suggested-by: Mark Brown
> Signed-off-by: Maxime Ripard
Applied to drm-misc-fixes
Maxime
signature.asc
Description: PGP signature
Re: [PATCH 07/14] dt-bindings: media: i2c: Add A31 MIPI CSI-2 bindings documentation
On Tue, Oct 27, 2020 at 10:52:21AM +0100, Paul Kocialkowski wrote: > Hi, > > On Mon 26 Oct 20, 17:14, Maxime Ripard wrote: > > i2c? :) > > Oops, good catch! > > > On Fri, Oct 23, 2020 at 07:45:39PM +0200, Paul Kocialkowski wrote: > > > This introduces YAML bindings documentation for the A31 MIPI CSI-2 > > > controller. > > > > > > Signed-off-by: Paul Kocialkowski > > > --- > > > .../media/allwinner,sun6i-a31-mipi-csi2.yaml | 168 ++ > > > 1 file changed, 168 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml > > > > > > diff --git > > > a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml > > > > > > b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml > > > new file mode 100644 > > > index ..9adc0bc27033 > > > --- /dev/null > > > +++ > > > b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml > > > @@ -0,0 +1,168 @@ > > > +# SPDX-License-Identifier: GPL-2.0 > > > +%YAML 1.2 > > > +--- > > > +$id: > > > http://devicetree.org/schemas/media/allwinner,sun6i-a31-mipi-csi2.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Allwinner A31 MIPI CSI-2 Device Tree Bindings > > > + > > > +maintainers: > > > + - Paul Kocialkowski > > > + > > > +properties: > > > + compatible: > > > +oneOf: > > > + - const: allwinner,sun6i-a31-mipi-csi2 > > > + - items: > > > + - const: allwinner,sun8i-v3s-mipi-csi2 > > > + - const: allwinner,sun6i-a31-mipi-csi2 > > > + > > > + reg: > > > +maxItems: 1 > > > + > > > + interrupts: > > > +maxItems: 1 > > > + > > > + clocks: > > > +items: > > > + - description: Bus Clock > > > + - description: Module Clock > > > + > > > + clock-names: > > > +items: > > > + - const: bus > > > + - const: mod > > > + > > > + phys: > > > +items: > > > + - description: MIPI D-PHY > > > + > > > + phy-names: > > > +items: > > > + - const: dphy > > > + > > > + resets: > > > +maxItems: 1 > > > + > > > + # See ./video-interfaces.txt for details > > > + ports: > > > +type: object > > > + > > > +properties: > > > + port@0: > > > +type: object > > > +description: Input port, connect to a MIPI CSI-2 sensor > > > + > > > +properties: > > > + reg: > > > +const: 0 > > > + > > > + endpoint: > > > +type: object > > > + > > > +properties: > > > + remote-endpoint: true > > > + > > > + bus-type: > > > +const: 4 > > > + > > > + clock-lanes: > > > +maxItems: 1 > > > + > > > + data-lanes: > > > +minItems: 1 > > > +maxItems: 4 > > > + > > > +required: > > > + - bus-type > > > + - data-lanes > > > + - remote-endpoint > > > + > > > +additionalProperties: false > > > + > > > +required: > > > + - endpoint > > > + > > > +additionalProperties: false > > > + > > > + port@1: > > > +type: object > > > +description: Output port, connect to a CSI controller > > > + > > > +properties: > > > + reg: > > > +const: 1 > > > + > > > + endpoint: > > > +type: object > > > + > > > +properties: > > > + remote-endpoint: true > > > + > > > + bus-type: > > > +const: 4 > > > > That one seems a bit weird. If the input and output ports are using the > > same format, what is that "bridge" supposed to be doing? > > Fair enough. What this represents is th
Re: [PATCH 05/14] media: sun6i-csi: Only configure the interface data width for parallel
On Tue, Oct 27, 2020 at 10:31:19AM +0100, Paul Kocialkowski wrote:
> Hi,
>
> On Mon 26 Oct 20, 17:00, Maxime Ripard wrote:
> > On Fri, Oct 23, 2020 at 07:45:37PM +0200, Paul Kocialkowski wrote:
> > > Bits related to the interface data width do not have any effect when
> > > the CSI controller is taking input from the MIPI CSI-2 controller.
> >
> > I guess it would be clearer to mention that the data width is only
> > applicable for parallel here.
>
> Understood, will change the wording in the next version.
>
> > > In prevision of adding support for this case, set these bits
> > > conditionally so there is no ambiguity.
> > >
> > > Co-developed-by: Kévin L'hôpital
> > > Signed-off-by: Kévin L'hôpital
> > > Signed-off-by: Paul Kocialkowski
> > > ---
> > > .../platform/sunxi/sun6i-csi/sun6i_csi.c | 42 +++
> > > 1 file changed, 25 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > index 5d2389a5cd17..a876a05ea3c7 100644
> > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > @@ -378,8 +378,13 @@ static void sun6i_csi_setup_bus(struct sun6i_csi_dev
> > > *sdev)
> > > unsigned char bus_width;
> > > u32 flags;
> > > u32 cfg;
> > > + bool input_parallel = false;
> > > bool input_interlaced = false;
> > >
> > > + if (endpoint->bus_type == V4L2_MBUS_PARALLEL ||
> > > + endpoint->bus_type == V4L2_MBUS_BT656)
> > > + input_parallel = true;
> > > +
> > > if (csi->config.field == V4L2_FIELD_INTERLACED
> > > || csi->config.field == V4L2_FIELD_INTERLACED_TB
> > > || csi->config.field == V4L2_FIELD_INTERLACED_BT)
> > > @@ -395,6 +400,26 @@ static void sun6i_csi_setup_bus(struct sun6i_csi_dev
> > > *sdev)
> > >CSI_IF_CFG_HREF_POL_MASK | CSI_IF_CFG_FIELD_MASK |
> > >CSI_IF_CFG_SRC_TYPE_MASK);
> > >
> > > + if (input_parallel) {
> > > + switch (bus_width) {
> > > + case 8:
> > > + cfg |= CSI_IF_CFG_IF_DATA_WIDTH_8BIT;
> > > + break;
> > > + case 10:
> > > + cfg |= CSI_IF_CFG_IF_DATA_WIDTH_10BIT;
> > > + break;
> > > + case 12:
> > > + cfg |= CSI_IF_CFG_IF_DATA_WIDTH_12BIT;
> > > + break;
> > > + case 16: /* No need to configure DATA_WIDTH for 16bit */
> > > + break;
> > > + default:
> > > + dev_warn(sdev->dev, "Unsupported bus width: %u\n",
> > > + bus_width);
> > > + break;
> > > + }
> > > + }
> > > +
> > > if (input_interlaced)
> > > cfg |= CSI_IF_CFG_SRC_TYPE_INTERLACED;
> > > else
> > > @@ -440,23 +465,6 @@ static void sun6i_csi_setup_bus(struct sun6i_csi_dev
> > > *sdev)
> > > break;
> > > }
> > >
> > > - switch (bus_width) {
> > > - case 8:
> > > - cfg |= CSI_IF_CFG_IF_DATA_WIDTH_8BIT;
> > > - break;
> > > - case 10:
> > > - cfg |= CSI_IF_CFG_IF_DATA_WIDTH_10BIT;
> > > - break;
> > > - case 12:
> > > - cfg |= CSI_IF_CFG_IF_DATA_WIDTH_12BIT;
> > > - break;
> > > - case 16: /* No need to configure DATA_WIDTH for 16bit */
> > > - break;
> > > - default:
> > > - dev_warn(sdev->dev, "Unsupported bus width: %u\n", bus_width);
> > > - break;
> > > - }
> > > -
> >
> > Is there any reason to move it around?
>
> The main reason is cosmetics: input_parallel is introduced to match the
> already
> existing input_interlaced variable, so it made sense to me to have both of
> these
> conditionals one after the other instead of spreading them randomly.
>
> I can mention this in the commit log if you prefer.
Yeah, that would great
Maxime
signature.asc
Description: PGP signature
Re: [PATCH 02/14] phy: allwinner: phy-sun6i-mipi-dphy: Support D-PHY Rx mode for MIPI CSI-2
Hi,
On Tue, Oct 27, 2020 at 10:23:26AM +0100, Paul Kocialkowski wrote:
> On Mon 26 Oct 20, 16:38, Maxime Ripard wrote:
> > On Fri, Oct 23, 2020 at 07:45:34PM +0200, Paul Kocialkowski wrote:
> > > The Allwinner A31 D-PHY supports both Rx and Tx modes. While the latter
> > > is already supported and used for MIPI DSI this adds support for the
> > > former, to be used with MIPI CSI-2.
> > >
> > > This implementation is inspired by the Allwinner BSP implementation.
> >
> > Mentionning which BSP you took this from would be helpful
>
> Sure! It's from the Github repo linked from https://linux-sunxi.org/V3s.
> Would you like that I mention this URL explicitly or would it be enough to
> mention "Allwinner's V3s Linux SDK" as they seem to call it?
Yeah, that would be great
> > > +static int sun6i_dphy_rx_power_on(struct sun6i_dphy *dphy)
> > > +{
> > > + /* Physical clock rate is actually half of symbol rate with DDR. */
> > > + unsigned long mipi_symbol_rate = dphy->config.hs_clk_rate;
> > > + unsigned long dphy_clk_rate;
> > > + unsigned int rx_dly;
> > > + unsigned int lprst_dly;
> > > + u32 value;
> > > +
> > > + dphy_clk_rate = clk_get_rate(dphy->mod_clk);
> > > + if (!dphy_clk_rate)
> > > + return -1;
> >
> > Returning -1 is weird here?
>
> What do you think would be a more appropriate error code to return?
> It looks like some other drivers return -EINVAL when that happens (but many
> don't do the check).
Yeah, EINVAL at least is better than ENOPERM
> > > +
> > > + /* Hardcoded timing parameters from the Allwinner BSP. */
> > > + regmap_write(dphy->regs, SUN6I_DPHY_RX_TIME0_REG,
> > > + SUN6I_DPHY_RX_TIME0_HS_RX_SYNC(255) |
> > > + SUN6I_DPHY_RX_TIME0_HS_RX_CLK_MISS(255) |
> > > + SUN6I_DPHY_RX_TIME0_LP_RX(255));
> > > +
> > > + /*
> > > + * Formula from the Allwinner BSP, with hardcoded coefficients
> > > + * (probably internal divider/multiplier).
> > > + */
> > > + rx_dly = 8 * (unsigned int)(dphy_clk_rate / (mipi_symbol_rate / 8));
> > > +
> > > + /*
> > > + * The Allwinner BSP has an alternative formula for LP_RX_ULPS_WP:
> > > + * lp_ulps_wp_cnt = lp_ulps_wp_ms * lp_clk / 1000
> > > + * but does not use it and hardcodes 255 instead.
> > > + */
> > > + regmap_write(dphy->regs, SUN6I_DPHY_RX_TIME1_REG,
> > > + SUN6I_DPHY_RX_TIME1_RX_DLY(rx_dly) |
> > > + SUN6I_DPHY_RX_TIME1_LP_RX_ULPS_WP(255));
> > > +
> > > + /* HS_RX_ANA0 value is hardcoded in the Allwinner BSP. */
> > > + regmap_write(dphy->regs, SUN6I_DPHY_RX_TIME2_REG,
> > > + SUN6I_DPHY_RX_TIME2_HS_RX_ANA0(4));
> > > +
> > > + /*
> > > + * Formula from the Allwinner BSP, with hardcoded coefficients
> > > + * (probably internal divider/multiplier).
> > > + */
> > > + lprst_dly = 4 * (unsigned int)(dphy_clk_rate / (mipi_symbol_rate / 2));
> > > +
> > > + regmap_write(dphy->regs, SUN6I_DPHY_RX_TIME3_REG,
> > > + SUN6I_DPHY_RX_TIME3_LPRST_DLY(lprst_dly));
> > > +
> > > + /* Analog parameters are hardcoded in the Allwinner BSP. */
> > > + regmap_write(dphy->regs, SUN6I_DPHY_ANA0_REG,
> > > + SUN6I_DPHY_ANA0_REG_PWS |
> > > + SUN6I_DPHY_ANA0_REG_SLV(7) |
> > > + SUN6I_DPHY_ANA0_REG_SFB(2));
> > > +
> > > + regmap_write(dphy->regs, SUN6I_DPHY_ANA1_REG,
> > > + SUN6I_DPHY_ANA1_REG_SVTT(4));
> > > +
> > > + regmap_write(dphy->regs, SUN6I_DPHY_ANA4_REG,
> > > + SUN6I_DPHY_ANA4_REG_DMPLVC |
> > > + SUN6I_DPHY_ANA4_REG_DMPLVD(1));
> > > +
> > > + regmap_write(dphy->regs, SUN6I_DPHY_ANA2_REG,
> > > + SUN6I_DPHY_ANA2_REG_ENIB);
> > > +
> > > + regmap_write(dphy->regs, SUN6I_DPHY_ANA3_REG,
> > > + SUN6I_DPHY_ANA3_EN_LDOR |
> > > + SUN6I_DPHY_ANA3_EN_LDOC |
> > > + SUN6I_DPHY_ANA3_EN_LDOD);
> > > +
> > > + /*
> > > + * Delay comes from the Allwinner BSP, likely for internal regulator
> > > + * ramp-up.
> > > + */
> > > + udelay(3);
> > > +
> > > + value = SUN6I_DPHY_RX_CTL_EN_DBC | SUN6I_DPHY_RX_CTL_RX_CLK_FORCE;
> > > +
> > > + /*
> > > + * Rx data lane force-enable bits are
Re: [PATCH v8 07/14] ASoC: sun4i-i2s: Fix setting of FIFO modes
On Mon, Oct 26, 2020 at 07:52:32PM +0100, Clément Péron wrote: > From: Samuel Holland > > Because SUN4I_I2S_FIFO_CTRL_REG is volatile, writes done while the > regmap is cache-only are ignored. To work around this, move the > configuration to a callback that runs while the ASoC core has a > runtime PM reference to the device. > > Signed-off-by: Samuel Holland > Reviewed-by: Chen-Yu Tsai > Signed-off-by: Clément Péron Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH v8 04/14] ASoC: sun4i-i2s: Set sign extend sample
On Mon, Oct 26, 2020 at 07:52:29PM +0100, Clément Péron wrote: > From: Marcus Cooper > > On the newer SoCs such as the H3 and A64 this is set by default > to transfer a 0 after each sample in each slot. However the A10 > and A20 SoCs that this driver was developed on had a default > setting where it padded the audio gain with zeros. > > This isn't a problem while we have only support for 16bit audio > but with larger sample resolution rates in the pipeline then SEXT > bits should be cleared so that they also pad at the LSB. Without > this the audio gets distorted. > > Set sign extend sample for all the sunxi generations even if they > are not affected. This will keep consistency and avoid relying on > default. > > Signed-off-by: Marcus Cooper > Reviewed-by: Chen-Yu Tsai > Signed-off-by: Clément Péron Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH v8 03/14] ASoC: sun4i-i2s: Change get_sr() and get_wss() to be more explicit
On Mon, Oct 26, 2020 at 07:52:28PM +0100, Clément Péron wrote:
> We are actually using a complex formula to just return a bunch of
> simple values. Also this formula is wrong for sun4i when calling
> get_wss() the function return 4 instead of 3.
>
> Replace this with a simpler switch case.
>
> Also drop the i2s params which is unused and return a simple int as
> returning an error code could be out of range for an s8 and there is
> no optim to return a s8 here.
>
> Fixes: 619c15f7fac9 ("ASoC: sun4i-i2s: Change SR and WSS computation")
> Reviewed-by: Chen-Yu Tsai
> Signed-off-by: Clément Péron
Acked-by: Maxime Ripard
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v8 01/14] ASoC: sun4i-i2s: Change set_chan_cfg() params
On Mon, Oct 26, 2020 at 07:52:26PM +0100, Clément Péron wrote:
> As slots and slot_width can be set manually using set_tdm().
> These values are then kept in sun4i_i2s struct.
> So we need to check if these values are set or not.
>
> This is not done actually and will trigger a bug.
> For example, if we set to the simple soundcard in the device-tree
> dai-tdm-slot-width = <32> and then start a stream using S16_LE,
> currently we would calculate BCLK for 32-bit slots, but program
> lrck_period for 16-bit slots, making the sample rate double what we
> expected.
>
> To fix this, we need to check if these values are set or not but as
> this logic is already done by the caller. Avoid duplicating this
> logic and just pass the required values as params to set_chan_cfg().
>
> Suggested-by: Samuel Holland
> Signed-off-by: Clément Péron
> ---
> sound/soc/sunxi/sun4i-i2s.c | 33 ++---
> 1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index f23ff29e7c1d..6c10f810b114 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -162,8 +162,9 @@ struct sun4i_i2s_quirks {
> unsigned long (*get_bclk_parent_rate)(const struct sun4i_i2s *);
> s8 (*get_sr)(const struct sun4i_i2s *, int);
> s8 (*get_wss)(const struct sun4i_i2s *, int);
> - int (*set_chan_cfg)(const struct sun4i_i2s *,
> - const struct snd_pcm_hw_params *);
> + int (*set_chan_cfg)(const struct sun4i_i2s *i2s,
> + unsigned int channels, unsigned int slots,
> + unsigned int slot_width);
> int (*set_fmt)(const struct sun4i_i2s *, unsigned int);
> };
>
> @@ -399,10 +400,9 @@ static s8 sun8i_i2s_get_sr_wss(const struct sun4i_i2s
> *i2s, int width)
> }
>
> static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> - const struct snd_pcm_hw_params *params)
> + unsigned int channels, unsigned int slots,
> + unsigned int slot_width)
> {
> - unsigned int channels = params_channels(params);
> -
> /* Map the channels for playback and capture */
> regmap_write(i2s->regmap, SUN4I_I2S_TX_CHAN_MAP_REG, 0x76543210);
> regmap_write(i2s->regmap, SUN4I_I2S_RX_CHAN_MAP_REG, 0x3210);
> @@ -419,15 +419,11 @@ static int sun4i_i2s_set_chan_cfg(const struct
> sun4i_i2s *i2s,
> }
>
> static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> - const struct snd_pcm_hw_params *params)
> + unsigned int channels, unsigned int slots,
> + unsigned int slot_width)
> {
> - unsigned int channels = params_channels(params);
> - unsigned int slots = channels;
> unsigned int lrck_period;
>
> - if (i2s->slots)
> - slots = i2s->slots;
> -
> /* Map the channels for playback and capture */
> regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_MAP_REG, 0x76543210);
> regmap_write(i2s->regmap, SUN8I_I2S_RX_CHAN_MAP_REG, 0x76543210);
> @@ -452,11 +448,11 @@ static int sun8i_i2s_set_chan_cfg(const struct
> sun4i_i2s *i2s,
> case SND_SOC_DAIFMT_DSP_B:
> case SND_SOC_DAIFMT_LEFT_J:
> case SND_SOC_DAIFMT_RIGHT_J:
> - lrck_period = params_physical_width(params) * slots;
> + lrck_period = slot_width * slots;
> break;
>
> case SND_SOC_DAIFMT_I2S:
> - lrck_period = params_physical_width(params);
> + lrck_period = slot_width;
> break;
>
> default:
> @@ -480,9 +476,16 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream
> *substream,
> {
> struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> unsigned int word_size = params_width(params);
> - unsigned int slot_width = params_physical_width(params);
> unsigned int channels = params_channels(params);
> +
> + /*
> + * Here and in set_chan_cfg(), "slots" means channels per frame +
> + * padding slots, regardless of format. "slot_width" means bits
> + * per sample + padding bits, regardless of format.
> + */
> unsigned int slots = channels;
> + unsigned int slot_width = params_physical_width(params);
> +
what I meant was to put that comment next to the function pointer in the
structure sun4i_i2s_quirks, it would be fairly easy to miss here.
With that fixed,
Acked-by: Maxime Ripard
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v8 02/14] ASoC: sun4i-i2s: Add support for H6 I2S
On Mon, Oct 26, 2020 at 07:52:27PM +0100, Clément Péron wrote: > From: Jernej Skrabec > > H6 I2S is very similar to that in H3, except it supports up to 16 > channels. > > Signed-off-by: Jernej Skrabec > Signed-off-by: Marcus Cooper > Reviewed-by: Chen-Yu Tsai > Signed-off-by: Clément Péron Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH 1/1] drm/vc4: drv: Add error handding for bind
Hi, On Tue, Oct 27, 2020 at 01:14:42PM +0900, Hoegeun Kwon wrote: > There is a problem that if vc4_drm bind fails, a memory leak occurs on > the drm_property_create side. Add error handding for drm_mode_config. > > Signed-off-by: Hoegeun Kwon Applied, thanks! Maxime signature.asc Description: PGP signature
Re: linux-next: build failure after merge of the sunxi tree
Hi Stephen,
On Tue, Oct 27, 2020 at 10:42:20AM +1100, Stephen Rothwell wrote:
> Hi all,
>
> After merging the sunxi tree, today's linux-next build (arm
> multi_v7_defconfig) failed like this:
>
> arch/arm/boot/dts/sun8i-h3-zeropi.dts:53.25-63.4: ERROR (phandle_references):
> /gmac-3v3: Reference to non-existent node or label "gmac_power_pin_nanopi"
>
> ERROR: Input tree has errors, aborting (use -f to force output)
>
> Caused by commit
>
> 89cfb6d76fdc ("ARM: dts: sun8i: add FriendlyArm ZeroPi support")
>
> I have reverted that commit for today.
Sorry for that, Yu-Tung sent a fix for it that is now in my branch for
next, so all should be good tomorrow.
Thanks!
Maxime
signature.asc
Description: PGP signature
Re: [PATCH] ARM: dts: sun8i: zeropi: remove useless pinctrl properties
On Tue, Oct 27, 2020 at 06:12:58PM +0800, Yu-Tung Chang wrote: > Signed-off-by: Yu-Tung Chang Ah so it was to fix the build breakage in next. I've squashed it into your previous patch. Generally speaking though, a commit log is needed Thanks! Maxime signature.asc Description: PGP signature
[PATCH] drm/vc4: hdmi: Avoid sleeping in atomic context
When running the trigger hook, ALSA by default will take a spinlock, and
thus will run the trigger hook in atomic context.
However, our HDMI driver will send the infoframes as part of the trigger
hook, and part of that process is to wait for a bit to be cleared for up to
100ms. To be nicer to the system, that wait has some usleep_range that
interact poorly with the atomic context.
There's several ways we can fix this, but the more obvious one is to make
ALSA take a mutex instead by setting the nonatomic flag on the DAI link.
That doesn't work though, since now the cyclic callback installed by the
dmaengine helpers in ALSA will take a mutex, while that callback is run by
dmaengine's virt-chan code in a tasklet where sleeping is not allowed
either.
Given the delay we need to poll the bit for, changing the usleep_range for
a udelay and keep running it from a context where interrupts are disabled
is not really a good option either.
However, we can move the infoframe setup code in the hw_params hook, like
is usually done in other HDMI controllers, that isn't protected by a
spinlock and thus where we can sleep. Infoframes will be sent on a regular
basis anyway, and since hw_params is where the audio parameters that end up
in the infoframes are setup, this also makes a bit more sense.
Fixes: bb7d78568814 ("drm/vc4: Add HDMI audio support")
Suggested-by: Mark Brown
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/vc4/vc4_hdmi.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 74da7c00ecd0..ec3ba3ecd32a 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1077,6 +1077,7 @@ static int vc4_hdmi_audio_hw_params(struct
snd_pcm_substream *substream,
struct snd_soc_dai *dai)
{
struct vc4_hdmi *vc4_hdmi = dai_to_hdmi(dai);
+ struct drm_encoder *encoder = _hdmi->encoder.base.base;
struct device *dev = _hdmi->pdev->dev;
u32 audio_packet_config, channel_mask;
u32 channel_map;
@@ -1136,6 +1137,8 @@ static int vc4_hdmi_audio_hw_params(struct
snd_pcm_substream *substream,
HDMI_WRITE(HDMI_AUDIO_PACKET_CONFIG, audio_packet_config);
vc4_hdmi_set_n_cts(vc4_hdmi);
+ vc4_hdmi_set_audio_infoframe(encoder);
+
return 0;
}
@@ -1143,11 +1146,9 @@ static int vc4_hdmi_audio_trigger(struct
snd_pcm_substream *substream, int cmd,
struct snd_soc_dai *dai)
{
struct vc4_hdmi *vc4_hdmi = dai_to_hdmi(dai);
- struct drm_encoder *encoder = _hdmi->encoder.base.base;
switch (cmd) {
case SNDRV_PCM_TRIGGER_START:
- vc4_hdmi_set_audio_infoframe(encoder);
vc4_hdmi->audio.streaming = true;
if (vc4_hdmi->variant->phy_rng_enable)
--
2.26.2
Re: [PATCH v4 1/1] ARM: dts: sun8i: add FriendlyArm ZeroPi support
On Tue, Oct 27, 2020 at 03:16:48PM +0800, Yu-Tung Chang wrote: > The ZeroPi is another fun board developed > by FriendlyELEC for makers, > hobbyists and fans. > > ZeroPi key features > - Allwinner H3, Quad-core Cortex-A7@1.2GHz > - 256MB/512MB DDR3 RAM > - microsd slot > - 10/100/1000Mbps Ethernet > - Debug Serial Port > - DC 5V/2A power-supply > > Signed-off-by: Yu-Tung Chang > Signed-off-by: Maxime Ripard > Link: https://lore.kernel.org/r/20201026073536.13617-2-mtw...@gmail.com Isn't it the patch that you sent and I merged yesterday? If you have some changes to it, please send those changes in a patch, not the full thing again. Maxime signature.asc Description: PGP signature
Re: [PATCH 3/3] mmc: sunxi: drop of_match_ptr from of_device_id table
On Mon, Oct 26, 2020 at 04:38:10PM +0100, Krzysztof Kozlowski wrote: > The driver can match only via DT table so it should be always used and > the of_match_ptr does not have any sense (this also allows ACPI > matching via PRP0001, even though it is not relevant for sunxi). This > fixes compile warning: > > drivers/mmc/host/sunxi-mmc.c:1181:34: warning: ‘sunxi_mmc_of_match’ > defined but not used [-Wunused-const-variable=] > > Reported-by: kernel test robot > Signed-off-by: Krzysztof Kozlowski Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH 00/14] Allwinner MIPI CSI-2 support for A31/V3s/A83T
On Fri, Oct 23, 2020 at 07:45:32PM +0200, Paul Kocialkowski wrote: > This series introduces support for MIPI CSI-2, with the A31 controller that is > found on most SoCs (A31, V3s and probably V5) as well as the A83T-specific > controller. While the former uses the same MIPI D-PHY that is already > supported > for DSI, the latter embeds its own D-PHY. > > In order to distinguish the use of the D-PHY between Rx mode (for MIPI CSI-2) > and Tx mode (for MIPI DSI), a submode is introduced for D-PHY in the PHY API. > This allows adding Rx support in the A31 D-PHY driver. > > A few changes and fixes are applied to the A31 CSI controller driver, in order > to support the MIPI CSI-2 use-case. > > Follows is the V4L2 device topology representing the interactions between > the MIPI CSI-2 sensor, the MIPI CSI-2 controller (which controls the D-PHY) > and the CSI controller: > - entity 1: sun6i-csi (1 pad, 1 link) > type Node subtype V4L flags 0 > device node name /dev/video0 > pad0: Sink > <- "sun6i-mipi-csi2":1 [ENABLED,IMMUTABLE] > > - entity 5: sun6i-mipi-csi2 (2 pads, 2 links) > type V4L2 subdev subtype Unknown flags 0 > pad0: Sink > <- "ov5648 0-0036":0 [ENABLED,IMMUTABLE] > pad1: Source > -> "sun6i-csi":0 [ENABLED,IMMUTABLE] > > - entity 8: ov5648 0-0036 (1 pad, 1 link) > type V4L2 subdev subtype Sensor flags 0 > device node name /dev/v4l-subdev0 > pad0: Source > [fmt:SBGGR8_1X8/640x480@1/30 field:none colorspace:raw > xfer:none ycbcr:601 quantization:full-range] > -> "sun6i-mipi-csi2":0 [ENABLED,IMMUTABLE] > > Happy reviewing! I mentioned it to Kevin in the first version, but you should have a v4l2-compliance run here. If you have some time, it would be great to run libcamera as well. Maxime signature.asc Description: PGP signature
Re: [PATCH 12/14] media: sunxi: Add support for the A83T MIPI CSI-2 controller
On Fri, Oct 23, 2020 at 07:45:44PM +0200, Paul Kocialkowski wrote: > The A83T supports MIPI CSI-2 with a composite controller, covering both the > protocol logic and the D-PHY implementation. This controller seems to be found > on the A83T only and probably was abandonned since. > > This implementation splits the protocol and D-PHY registers and uses the PHY > framework internally. The D-PHY is not registered as a standalone PHY driver > since it cannot be used with any other controller. > > There are a few notable points about the controller: > - The initialisation sequence involes writing specific magic init values that > do not seem to make any particular sense given the concerned register > fields. > - Interrupts appear to be hitting regardless of the interrupt mask registers, > which can cause a serious flood when transmission errors occur. Ah, so it's a separate driver too. > This work is based on the first version of the driver submitted by > Kévin L'hôpital, which was adapted to mainline from the Allwinner BSP. > This version integrates MIPI CSI-2 support as a standalone V4L2 subdev > instead of merging it in the sun6i-csi driver. > > It was tested on a Banana Pi M3 board with an OV8865 sensor in a 4-lane > configuration. Co-developped-by and SoB from Kevin? Looking at the driver, the same comments from the v3s apply there Maxime signature.asc Description: PGP signature
Re: [PATCH 14/14] media: sunxi: sun8i-a83t-mipi-csi2: Avoid using the (unsolicited) interrupt
On Fri, Oct 23, 2020 at 07:45:46PM +0200, Paul Kocialkowski wrote: > The A83T MIPI CSI-2 apparently produces interrupts regardless of the mask > registers, for example when a transmission error occurs. > > This generates quite a flood when unsolicited interrupts are received on > each received frame. As a result, disable the interrupt for now since > we are not currently using it for error reporting. > > Signed-off-by: Paul Kocialkowski This should be merged into the driver patch Maxime signature.asc Description: PGP signature
Re: [PATCH 11/14] dt-bindings: media: i2c: Add A83T MIPI CSI-2 bindings documentation
On Fri, Oct 23, 2020 at 07:45:43PM +0200, Paul Kocialkowski wrote: > This introduces YAML bindings documentation for the A83T MIPI CSI-2 > controller. > > Signed-off-by: Paul Kocialkowski What is the difference with the a31/v3s one? > --- > .../media/allwinner,sun8i-a83t-mipi-csi2.yaml | 158 ++ > 1 file changed, 158 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/media/allwinner,sun8i-a83t-mipi-csi2.yaml > > diff --git > a/Documentation/devicetree/bindings/media/allwinner,sun8i-a83t-mipi-csi2.yaml > b/Documentation/devicetree/bindings/media/allwinner,sun8i-a83t-mipi-csi2.yaml > new file mode 100644 > index ..2384ae4e7be0 > --- /dev/null > +++ > b/Documentation/devicetree/bindings/media/allwinner,sun8i-a83t-mipi-csi2.yaml > @@ -0,0 +1,158 @@ > +# SPDX-License-Identifier: GPL-2.0 > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/allwinner,sun8i-a83t-mipi-csi2.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Allwinner A83T MIPI CSI-2 Device Tree Bindings > + > +maintainers: > + - Paul Kocialkowski > + > +properties: > + compatible: > +const: allwinner,sun8i-a83t-mipi-csi2 > + > + reg: > +maxItems: 1 > + > + interrupts: > +maxItems: 1 > + > + clocks: > +items: > + - description: Bus Clock > + - description: Module Clock > + - description: MIPI-specific Clock > + - description: Misc CSI Clock > + > + clock-names: > +items: > + - const: bus > + - const: mod > + - const: mipi > + - const: misc If it's only due to the clock, it's soemething you can deal with in the first schema, there's no need to duplicate them. Maxime signature.asc Description: PGP signature
Re: [PATCH 10/14] ARM: dts: sun8i: v3s: Add MIPI D-PHY and MIPI CSI-2 interface nodes
On Fri, Oct 23, 2020 at 07:45:42PM +0200, Paul Kocialkowski wrote: > MIPI CSI-2 is supported on the V3s with an A31 controller, which seems > to be used on all Allwinner chips supporting it, except for the A83T. > The controller is connected to CSI0 through fwnode endpoints. > The mipi_csi2_in port node is provided to connect MIPI CSI-2 sensors. > > The D-PHY part is the same that already drives DSI, but used in Rx mode. > > Signed-off-by: Paul Kocialkowski Especially since you haven't tested CSI0 without MIPI-CSI, you can squash that patch into the previous one. Maxime signature.asc Description: PGP signature
Re: [PATCH 08/14] media: sunxi: Add support for the A31 MIPI CSI-2 controller
On Fri, Oct 23, 2020 at 07:45:40PM +0200, Paul Kocialkowski wrote:
> The A31 MIPI CSI-2 controller is a dedicated MIPI CSI-2 controller
> found on Allwinner SoCs such as the A31 and V3/V3s.
>
> It is a standalone block, connected to the CSI controller on one side
> and to the MIPI D-PHY block on the other. It has a dedicated address
> space, interrupt line and clock.
>
> Currently, the MIPI CSI-2 controller is hard-tied to a specific CSI
> controller (CSI0) but newer SoCs (such as the V5) may allow switching
> MIPI CSI-2 controllers between CSI controllers.
>
> It is represented as a V4L2 subdev to the CSI controller and takes a
> MIPI CSI-2 sensor as its own subdev, all using the fwnode graph and
> media controller API.
>
> Signed-off-by: Paul Kocialkowski
> ---
> drivers/media/platform/sunxi/Kconfig | 1 +
> drivers/media/platform/sunxi/Makefile | 1 +
> .../platform/sunxi/sun6i-mipi-csi2/Kconfig| 11 +
> .../platform/sunxi/sun6i-mipi-csi2/Makefile | 4 +
> .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c | 635 ++
> .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h | 116
> 6 files changed, 768 insertions(+)
> create mode 100644 drivers/media/platform/sunxi/sun6i-mipi-csi2/Kconfig
> create mode 100644 drivers/media/platform/sunxi/sun6i-mipi-csi2/Makefile
> create mode 100644
> drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c
> create mode 100644
> drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h
>
> diff --git a/drivers/media/platform/sunxi/Kconfig
> b/drivers/media/platform/sunxi/Kconfig
> index 7151cc249afa..9684e07454ad 100644
> --- a/drivers/media/platform/sunxi/Kconfig
> +++ b/drivers/media/platform/sunxi/Kconfig
> @@ -2,3 +2,4 @@
>
> source "drivers/media/platform/sunxi/sun4i-csi/Kconfig"
> source "drivers/media/platform/sunxi/sun6i-csi/Kconfig"
> +source "drivers/media/platform/sunxi/sun6i-mipi-csi2/Kconfig"
> diff --git a/drivers/media/platform/sunxi/Makefile
> b/drivers/media/platform/sunxi/Makefile
> index fc537c9f5ca9..887a7cae8fca 100644
> --- a/drivers/media/platform/sunxi/Makefile
> +++ b/drivers/media/platform/sunxi/Makefile
> @@ -2,5 +2,6 @@
>
> obj-y+= sun4i-csi/
> obj-y+= sun6i-csi/
> +obj-y+= sun6i-mipi-csi2/
> obj-y+= sun8i-di/
> obj-y+= sun8i-rotate/
> diff --git a/drivers/media/platform/sunxi/sun6i-mipi-csi2/Kconfig
> b/drivers/media/platform/sunxi/sun6i-mipi-csi2/Kconfig
> new file mode 100644
> index ..7033bda483b4
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun6i-mipi-csi2/Kconfig
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config VIDEO_SUN6I_MIPI_CSI2
> + tristate "Allwinner A31 MIPI CSI-2 Controller Driver"
> + depends on VIDEO_V4L2 && COMMON_CLK
> + depends on ARCH_SUNXI || COMPILE_TEST
> + select MEDIA_CONTROLLER
> + select VIDEO_V4L2_SUBDEV_API
> + select REGMAP_MMIO
> + select V4L2_FWNODE
> + help
> +Support for the Allwinner A31 MIPI CSI-2 Controller.
> diff --git a/drivers/media/platform/sunxi/sun6i-mipi-csi2/Makefile
> b/drivers/media/platform/sunxi/sun6i-mipi-csi2/Makefile
> new file mode 100644
> index ..14e4e03818b5
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun6i-mipi-csi2/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +sun6i-mipi-csi2-y += sun6i_mipi_csi2.o
> +
> +obj-$(CONFIG_VIDEO_SUN6I_MIPI_CSI2) += sun6i-mipi-csi2.o
> diff --git a/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c
> b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c
> new file mode 100644
> index ..ce89c35f5b86
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c
> @@ -0,0 +1,635 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2020 Bootlin
> + * Author: Paul Kocialkowski
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "sun6i_mipi_csi2.h"
> +
> +#define MODULE_NAME "sun6i-mipi-csi2"
> +
> +/* Core */
> +
> +static irqreturn_t sun6i_mipi_csi2_isr(int irq, void *dev_id)
> +{
> + struct sun6i_mipi_csi2_dev *cdev = (struct sun6i_mipi_csi2_dev *)dev_id;
> + struct regmap *regmap = cdev->regmap;
> + u32 pending;
> +
> + WARN_ONCE(1, MODULE_NAME
> + ": Unsolicited interrupt, an error likely occurred!\n");
> +
> + regmap_read(regmap, SUN6I_MIPI_CSI2_CH_INT_PD_REG, );
> + regmap_write(regmap, SUN6I_MIPI_CSI2_CH_INT_PD_REG, pending);
> +
> + /*
> + * The interrupt can be used to catch transmission errors.
> + * However, we currently lack plumbing for reporting that to the
> + * A31 CSI controller driver.
> + */
> +
> + return IRQ_HANDLED;
> +}
Uhh, what is this handler supposed to be doing? The warning will
Re: [PATCH 07/14] dt-bindings: media: i2c: Add A31 MIPI CSI-2 bindings documentation
i2c? :)
On Fri, Oct 23, 2020 at 07:45:39PM +0200, Paul Kocialkowski wrote:
> This introduces YAML bindings documentation for the A31 MIPI CSI-2
> controller.
>
> Signed-off-by: Paul Kocialkowski
> ---
> .../media/allwinner,sun6i-a31-mipi-csi2.yaml | 168 ++
> 1 file changed, 168 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml
>
> diff --git
> a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml
> b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml
> new file mode 100644
> index ..9adc0bc27033
> --- /dev/null
> +++
> b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml
> @@ -0,0 +1,168 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/allwinner,sun6i-a31-mipi-csi2.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Allwinner A31 MIPI CSI-2 Device Tree Bindings
> +
> +maintainers:
> + - Paul Kocialkowski
> +
> +properties:
> + compatible:
> +oneOf:
> + - const: allwinner,sun6i-a31-mipi-csi2
> + - items:
> + - const: allwinner,sun8i-v3s-mipi-csi2
> + - const: allwinner,sun6i-a31-mipi-csi2
> +
> + reg:
> +maxItems: 1
> +
> + interrupts:
> +maxItems: 1
> +
> + clocks:
> +items:
> + - description: Bus Clock
> + - description: Module Clock
> +
> + clock-names:
> +items:
> + - const: bus
> + - const: mod
> +
> + phys:
> +items:
> + - description: MIPI D-PHY
> +
> + phy-names:
> +items:
> + - const: dphy
> +
> + resets:
> +maxItems: 1
> +
> + # See ./video-interfaces.txt for details
> + ports:
> +type: object
> +
> +properties:
> + port@0:
> +type: object
> +description: Input port, connect to a MIPI CSI-2 sensor
> +
> +properties:
> + reg:
> +const: 0
> +
> + endpoint:
> +type: object
> +
> +properties:
> + remote-endpoint: true
> +
> + bus-type:
> +const: 4
> +
> + clock-lanes:
> +maxItems: 1
> +
> + data-lanes:
> +minItems: 1
> +maxItems: 4
> +
> +required:
> + - bus-type
> + - data-lanes
> + - remote-endpoint
> +
> +additionalProperties: false
> +
> +required:
> + - endpoint
> +
> +additionalProperties: false
> +
> + port@1:
> +type: object
> +description: Output port, connect to a CSI controller
> +
> +properties:
> + reg:
> +const: 1
> +
> + endpoint:
> +type: object
> +
> +properties:
> + remote-endpoint: true
> +
> + bus-type:
> +const: 4
That one seems a bit weird. If the input and output ports are using the
same format, what is that "bridge" supposed to be doing?
> +additionalProperties: false
> +
> +required:
> + - endpoint
> +
> +additionalProperties: false
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clocks
> + - clock-names
> + - resets
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +#include
> +#include
> +#include
> +
> +mipi_csi2: mipi-csi2@1cb1000 {
The unit name should be pretty standard, with the list here:
https://github.com/devicetree-org/devicetree-specification/blob/master/source/chapter2-devicetree-basics.rst#generic-names-recommendation
there's nothing really standing out for us in that list, but given that
there's dsi, we should stick with csi
Maxime
signature.asc
Description: PGP signature
Re: [PATCH 05/14] media: sun6i-csi: Only configure the interface data width for parallel
On Fri, Oct 23, 2020 at 07:45:37PM +0200, Paul Kocialkowski wrote:
> Bits related to the interface data width do not have any effect when
> the CSI controller is taking input from the MIPI CSI-2 controller.
I guess it would be clearer to mention that the data width is only
applicable for parallel here.
> In prevision of adding support for this case, set these bits
> conditionally so there is no ambiguity.
>
> Co-developed-by: Kévin L'hôpital
> Signed-off-by: Kévin L'hôpital
> Signed-off-by: Paul Kocialkowski
> ---
> .../platform/sunxi/sun6i-csi/sun6i_csi.c | 42 +++
> 1 file changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> index 5d2389a5cd17..a876a05ea3c7 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> @@ -378,8 +378,13 @@ static void sun6i_csi_setup_bus(struct sun6i_csi_dev
> *sdev)
> unsigned char bus_width;
> u32 flags;
> u32 cfg;
> + bool input_parallel = false;
> bool input_interlaced = false;
>
> + if (endpoint->bus_type == V4L2_MBUS_PARALLEL ||
> + endpoint->bus_type == V4L2_MBUS_BT656)
> + input_parallel = true;
> +
> if (csi->config.field == V4L2_FIELD_INTERLACED
> || csi->config.field == V4L2_FIELD_INTERLACED_TB
> || csi->config.field == V4L2_FIELD_INTERLACED_BT)
> @@ -395,6 +400,26 @@ static void sun6i_csi_setup_bus(struct sun6i_csi_dev
> *sdev)
>CSI_IF_CFG_HREF_POL_MASK | CSI_IF_CFG_FIELD_MASK |
>CSI_IF_CFG_SRC_TYPE_MASK);
>
> + if (input_parallel) {
> + switch (bus_width) {
> + case 8:
> + cfg |= CSI_IF_CFG_IF_DATA_WIDTH_8BIT;
> + break;
> + case 10:
> + cfg |= CSI_IF_CFG_IF_DATA_WIDTH_10BIT;
> + break;
> + case 12:
> + cfg |= CSI_IF_CFG_IF_DATA_WIDTH_12BIT;
> + break;
> + case 16: /* No need to configure DATA_WIDTH for 16bit */
> + break;
> + default:
> + dev_warn(sdev->dev, "Unsupported bus width: %u\n",
> + bus_width);
> + break;
> + }
> + }
> +
> if (input_interlaced)
> cfg |= CSI_IF_CFG_SRC_TYPE_INTERLACED;
> else
> @@ -440,23 +465,6 @@ static void sun6i_csi_setup_bus(struct sun6i_csi_dev
> *sdev)
> break;
> }
>
> - switch (bus_width) {
> - case 8:
> - cfg |= CSI_IF_CFG_IF_DATA_WIDTH_8BIT;
> - break;
> - case 10:
> - cfg |= CSI_IF_CFG_IF_DATA_WIDTH_10BIT;
> - break;
> - case 12:
> - cfg |= CSI_IF_CFG_IF_DATA_WIDTH_12BIT;
> - break;
> - case 16: /* No need to configure DATA_WIDTH for 16bit */
> - break;
> - default:
> - dev_warn(sdev->dev, "Unsupported bus width: %u\n", bus_width);
> - break;
> - }
> -
Is there any reason to move it around?
Maxime
signature.asc
Description: PGP signature
Re: [PATCH 03/14] media: sun6i-csi: Support an optional dedicated memory pool
On Fri, Oct 23, 2020 at 07:45:35PM +0200, Paul Kocialkowski wrote:
> This allows selecting a dedicated CMA memory pool (specified via
> device-tree) instead of the default one.
>
> Signed-off-by: Paul Kocialkowski
Why would that be needed?
> ---
> drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> index 28e89340fed9..5d2389a5cd17 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> @@ -16,6 +16,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -849,6 +850,12 @@ static int sun6i_csi_resource_request(struct
> sun6i_csi_dev *sdev,
> return PTR_ERR(sdev->regmap);
> }
>
> + ret = of_reserved_mem_device_init(>dev);
> + if (ret && ret != -ENODEV) {
> + dev_err(>dev, "Unable to init reserved memory\n");
> + return ret;
> + }
> +
> sdev->clk_mod = devm_clk_get(>dev, "mod");
If that clk_get or any subsequent function fail you'll end up leaking
whatever the initialization of the reserved memory has allocated
Maxime
signature.asc
Description: PGP signature
Re: [PATCH 02/14] phy: allwinner: phy-sun6i-mipi-dphy: Support D-PHY Rx mode for MIPI CSI-2
On Fri, Oct 23, 2020 at 07:45:34PM +0200, Paul Kocialkowski wrote:
> The Allwinner A31 D-PHY supports both Rx and Tx modes. While the latter
> is already supported and used for MIPI DSI this adds support for the
> former, to be used with MIPI CSI-2.
>
> This implementation is inspired by the Allwinner BSP implementation.
Mentionning which BSP you took this from would be helpful
> Signed-off-by: Paul Kocialkowski
> ---
> drivers/phy/allwinner/phy-sun6i-mipi-dphy.c | 164 +++-
> 1 file changed, 160 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c
> b/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c
> index 1fa761ba6cbb..8bcd4bb79f60 100644
> --- a/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c
> +++ b/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c
> @@ -24,6 +24,14 @@
> #define SUN6I_DPHY_TX_CTL_REG0x04
> #define SUN6I_DPHY_TX_CTL_HS_TX_CLK_CONT BIT(28)
>
> +#define SUN6I_DPHY_RX_CTL_REG0x08
> +#define SUN6I_DPHY_RX_CTL_EN_DBC BIT(31)
> +#define SUN6I_DPHY_RX_CTL_RX_CLK_FORCE BIT(24)
> +#define SUN6I_DPHY_RX_CTL_RX_D3_FORCEBIT(23)
> +#define SUN6I_DPHY_RX_CTL_RX_D2_FORCEBIT(22)
> +#define SUN6I_DPHY_RX_CTL_RX_D1_FORCEBIT(21)
> +#define SUN6I_DPHY_RX_CTL_RX_D0_FORCEBIT(20)
> +
It's hard to tell from the diff, but it looks like you aligned the
BIT(..) with the register?
If so, you should follow the what the rest of this driver (ie, one more
indentation for register values).
> #define SUN6I_DPHY_TX_TIME0_REG 0x10
> #define SUN6I_DPHY_TX_TIME0_HS_TRAIL(n) (((n) & 0xff) << 24)
> #define SUN6I_DPHY_TX_TIME0_HS_PREPARE(n)(((n) & 0xff) << 16)
> @@ -44,12 +52,29 @@
> #define SUN6I_DPHY_TX_TIME4_HS_TX_ANA1(n)(((n) & 0xff) << 8)
> #define SUN6I_DPHY_TX_TIME4_HS_TX_ANA0(n)((n) & 0xff)
>
> +#define SUN6I_DPHY_RX_TIME0_REG 0x30
> +#define SUN6I_DPHY_RX_TIME0_HS_RX_SYNC(n)(((n) & 0xff) << 24)
> +#define SUN6I_DPHY_RX_TIME0_HS_RX_CLK_MISS(n)(((n) & 0xff) << 16)
> +#define SUN6I_DPHY_RX_TIME0_LP_RX(n) (((n) & 0xff) << 8)
> +
> +#define SUN6I_DPHY_RX_TIME1_REG 0x34
> +#define SUN6I_DPHY_RX_TIME1_RX_DLY(n)(((n) & 0xfff) << 20)
> +#define SUN6I_DPHY_RX_TIME1_LP_RX_ULPS_WP(n) ((n) & 0xf)
> +
> +#define SUN6I_DPHY_RX_TIME2_REG 0x38
> +#define SUN6I_DPHY_RX_TIME2_HS_RX_ANA1(n)(((n) & 0xff) << 8)
> +#define SUN6I_DPHY_RX_TIME2_HS_RX_ANA0(n)((n) & 0xff)
> +
> +#define SUN6I_DPHY_RX_TIME3_REG 0x40
> +#define SUN6I_DPHY_RX_TIME3_LPRST_DLY(n) (((n) & 0x) << 16)
> +
> #define SUN6I_DPHY_ANA0_REG 0x4c
> #define SUN6I_DPHY_ANA0_REG_PWS BIT(31)
> #define SUN6I_DPHY_ANA0_REG_DMPC BIT(28)
> #define SUN6I_DPHY_ANA0_REG_DMPD(n) (((n) & 0xf) << 24)
> #define SUN6I_DPHY_ANA0_REG_SLV(n) (((n) & 7) << 12)
> #define SUN6I_DPHY_ANA0_REG_DEN(n) (((n) & 0xf) << 8)
> +#define SUN6I_DPHY_ANA0_REG_SFB(n) (((n) & 3) << 2)
>
> #define SUN6I_DPHY_ANA1_REG 0x50
> #define SUN6I_DPHY_ANA1_REG_VTTMODE BIT(31)
> @@ -92,6 +117,8 @@ struct sun6i_dphy {
>
> struct phy *phy;
> struct phy_configure_opts_mipi_dphy config;
> +
> + int submode;
> };
>
> static int sun6i_dphy_init(struct phy *phy)
> @@ -105,6 +132,18 @@ static int sun6i_dphy_init(struct phy *phy)
> return 0;
> }
>
> +static int sun6i_dphy_set_mode(struct phy *phy, enum phy_mode mode, int
> submode)
> +{
> + struct sun6i_dphy *dphy = phy_get_drvdata(phy);
> +
> + if (mode != PHY_MODE_MIPI_DPHY)
> + return -EINVAL;
> +
> + dphy->submode = submode;
> +
> + return 0;
> +}
> +
> static int sun6i_dphy_configure(struct phy *phy, union phy_configure_opts
> *opts)
> {
> struct sun6i_dphy *dphy = phy_get_drvdata(phy);
> @@ -119,9 +158,8 @@ static int sun6i_dphy_configure(struct phy *phy, union
> phy_configure_opts *opts)
> return 0;
> }
>
> -static int sun6i_dphy_power_on(struct phy *phy)
> +static int sun6i_dphy_tx_power_on(struct sun6i_dphy *dphy)
> {
> - struct sun6i_dphy *dphy = phy_get_drvdata(phy);
> u8 lanes_mask = GENMASK(dphy->config.lanes - 1, 0);
>
> regmap_write(dphy->regs, SUN6I_DPHY_TX_CTL_REG,
> @@ -211,12 +249,129 @@ static int sun6i_dphy_power_on(struct phy *phy)
> return 0;
> }
>
> +static int sun6i_dphy_rx_power_on(struct sun6i_dphy *dphy)
> +{
> + /* Physical clock rate is actually half of symbol rate with DDR. */
> + unsigned long mipi_symbol_rate = dphy->config.hs_clk_rate;
> + unsigned long dphy_clk_rate;
> + unsigned int rx_dly;
> + unsigned int lprst_dly;
> + u32 value;
> +
> + dphy_clk_rate = clk_get_rate(dphy->mod_clk);
> + if (!dphy_clk_rate)
> + return -1;
Returning -1 is
Re: linux-next: manual merge of the sunxi tree with the arm-soc tree
Hi,
On Mon, Oct 26, 2020 at 08:54:21AM +1100, Stephen Rothwell wrote:
> On Thu, 8 Oct 2020 15:20:17 +0200 Maxime Ripard wrote:
> >
> > On Tue, Oct 06, 2020 at 02:56:37PM +1100, Stephen Rothwell wrote:
> > > Hi all,
> > >
> > > Today's linux-next merge of the sunxi tree got a conflict in:
> > >
> > > arch/arm64/boot/dts/allwinner/sun50i-a100.dtsi
> > >
> > > between commit:
> > >
> > > 0dea1794f3b4 ("arm64: allwinner: A100: add the basical Allwinner A100
> > > DTSI file")
> > >
> > > from the arm-soc tree and commit:
> > >
> > > 7e66a778cb8b ("arm64: allwinner: A100: add the basical Allwinner A100
> > > DTSI file")
> > >
> > > from the sunxi tree.
> > >
> > > These are 2 versions of the same patch. For now I am just using the
> > > version in the arm-soc tree ... please sort this out.
> >
> > The branch in arm-soc has a build breakage (that doesn't happen in
> > linux-next since the clk tree has the commit to fix it) so I sent a new
> > PR
> >
> > Once that PR is in arm-soc, I guess that merge issue will go away
>
> I am still getting the same conflict (but between Linus' tree and the
> sunxi tree). It looks like the sunxi tree has not been updated since
> October 6 ...
Yeah, the PR has never been picked up by arm-soc. I've pushed a new
branch based on 5.10-rc1 on our repo, it should solve the conflict.
Sorry for the inconvenience
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v3 1/1] ARM: dts: sun8i: add FriendlyArm ZeroPi support
Hi, On Mon, Oct 26, 2020 at 03:35:36PM +0800, Yu-Tung Chang wrote: > The ZeroPi is another fun board developed > by FriendlyELEC for makers, > hobbyists and fans. > > ZeroPi key features > - Allwinner H3, Quad-core Cortex-A7@1.2GHz > - 256MB/512MB DDR3 RAM > - microsd slot > - 10/100/1000Mbps Ethernet > - Debug Serial Port > - DC 5V/2A power-supply > > Signed-off-by: Yu-Tung Chang > --- > .../devicetree/bindings/arm/sunxi.yaml| 5 ++ > arch/arm/boot/dts/Makefile| 1 + > arch/arm/boot/dts/sun8i-h3-zeropi.dts | 87 +++ > 3 files changed, 93 insertions(+) > create mode 100644 arch/arm/boot/dts/sun8i-h3-zeropi.dts > > diff --git a/Documentation/devicetree/bindings/arm/sunxi.yaml > b/Documentation/devicetree/bindings/arm/sunxi.yaml > index efc9118233b4..9392a9a3f7e7 100644 > --- a/Documentation/devicetree/bindings/arm/sunxi.yaml > +++ b/Documentation/devicetree/bindings/arm/sunxi.yaml > @@ -246,6 +246,11 @@ properties: >- const: friendlyarm,nanopi-neo-plus2 >- const: allwinner,sun50i-h5 > > + - description: FriendlyARM ZeroPi > +items: > + - const: friendlyarm,zeropi > + - const: allwinner,sun50i-h3 > + There's a typo here, it's supposed to be sun8i-h3 instead. I've fixed it up and applied for 5.11. Thanks! Maxime signature.asc Description: PGP signature
Re: [PATCH 10/10] arm64: dts: allwinner: a64: bananapi-m64: Enable RGMII RX/TX delay on PHY
On Sun, Oct 25, 2020 at 12:25:15AM +0800, Chen-Yu Tsai wrote:
> From: Chen-Yu Tsai
>
> The Ethernet PHY on the Bananapi M64 has the RX and TX delays
> enabled on the PHY, using pull-ups on the RXDLY and TXDLY pins.
>
> Fix the phy-mode description to correct reflect this so that the
> implementation doesn't reconfigure the delays incorrectly. This
> happened with commit bbc4d71d6354 ("net: phy: realtek: fix rtl8211e
> rx/tx delay config").
>
> Fixes: e7295499903d ("arm64: allwinner: bananapi-m64: Enable dwmac-sun8i")
> Fixes: 94f442886711 ("arm64: dts: allwinner: A64: Restore EMAC changes")
> Signed-off-by: Chen-Yu Tsai
Applied all the patches, thanks!
Maxime
signature.asc
Description: PGP signature
Re: [PATCH] ARM: dts: sun8i: r40: bananapi-m2-ultra: Fix ethernet node
On Sun, Oct 25, 2020 at 09:19:49AM +0100, Jernej Skrabec wrote:
> Ethernet PHY on BananaPi M2 Ultra provides RX and TX delays. Fix
> ethernet node to reflect that fact.
>
> Fixes: c36fd5a48bd2 ("ARM: dts: sun8i: r40: bananapi-m2-ultra: Enable GMAC
> ethernet controller")
> Signed-off-by: Jernej Skrabec
Applied, thanks!
Maxime
signature.asc
Description: PGP signature
Re: [PATCH] arm64: dts: allwinner: h6: Pine H64: Fix ethernet node
On Fri, Oct 23, 2020 at 09:49:02PM +0200, Jernej Skrabec wrote:
> Ethernet PHY provides RX and TX delay on both models, A and B. Although
> schematic for model A suggests only TX delay, network never worked with
> such configuration.
>
> Fix ethernet node to reflect PHY delays.
>
> Fixes: 729e1ffcf47e ("arm64: allwinner: h6: add support for the Ethernet on
> Pine H64")
> Signed-off-by: Jernej Skrabec
Corentin sent the same patch last week. It's part of the fixes branch already.
maxime
signature.asc
Description: PGP signature
Re: [PATCH] arm64: dts: allwinner: h5: OrangePi PC2: Fix ethernet node
On Fri, Oct 23, 2020 at 08:48:58PM +0200, Jernej Skrabec wrote:
> RX and TX delay are provided by ethernet PHY. Reflect that in ethernet
> node.
>
> Fixes: 44a94c7ef989 ("arm64: dts: allwinner: H5: Restore EMAC changes")
> Signed-off-by: Jernej Skrabec
Applied, thanks
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v1] ARM: dts: sun8i: add FriendlyArm ZeroPi support
Hi,
Thanks for your patch
On Fri, Oct 23, 2020 at 05:09:08PM +0800, Yu-Tung Chang wrote:
> The ZeroPi is another fun board developed
> by FriendlyELEC for makers,
> hobbyists and fans.
>
> ZeroPi key features
> - Allwinner H3, Quad-core Cortex-A7@1.2GHz
> - 256MB/512MB DDR3 RAM
> - microsd slot
> - 10/100/1000Mbps Ethernet
> - Debug Serial Port
> - DC 5V/2A power-supply
>
> Signed-off-by: Yu-Tung Chang
> ---
> .../devicetree/bindings/arm/sunxi.yaml| 5 ++
> arch/arm/boot/dts/Makefile| 1 +
> arch/arm/boot/dts/sun8i-h3-zeropi.dts | 69 +++
> 3 files changed, 75 insertions(+)
> create mode 100644 arch/arm/boot/dts/sun8i-h3-zeropi.dts
>
> diff --git a/Documentation/devicetree/bindings/arm/sunxi.yaml
> b/Documentation/devicetree/bindings/arm/sunxi.yaml
> index efc9118233b4..9392a9a3f7e7 100644
> --- a/Documentation/devicetree/bindings/arm/sunxi.yaml
> +++ b/Documentation/devicetree/bindings/arm/sunxi.yaml
> @@ -246,6 +246,11 @@ properties:
>- const: friendlyarm,nanopi-neo-plus2
>- const: allwinner,sun50i-h5
>
> + - description: FriendlyARM ZeroPi
> +items:
> + - const: friendlyarm,zeropi
> + - const: allwinner,sun50i-h3
> +
>- description: Gemei G9 Tablet
> items:
>- const: gemei,g9
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 4572db3fa5ae..f05e54257947 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1187,6 +1187,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
> sun8i-h3-orangepi-plus2e.dtb \
> sun8i-h3-orangepi-zero-plus2.dtb \
> sun8i-h3-rervision-dvk.dtb \
> + sun8i-h3-zeropi.dtb \
> sun8i-h3-emlid-neutis-n5h3-devboard.dtb \
> sun8i-r16-bananapi-m2m.dtb \
> sun8i-r16-nintendo-nes-classic.dtb \
> diff --git a/arch/arm/boot/dts/sun8i-h3-zeropi.dts
> b/arch/arm/boot/dts/sun8i-h3-zeropi.dts
> new file mode 100644
> index ..388ad6b6da2b
> --- /dev/null
> +++ b/arch/arm/boot/dts/sun8i-h3-zeropi.dts
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (C) 2020 Yu-Tung Chang
> + */
> +
> +#include "sun8i-h3-nanopi.dtsi"
> +
> +/ {
> + model = "FriendlyARM ZeroPi";
> + compatible = "friendlyarm,zeropi", "allwinner,sun8i-h3";
> +
> + aliases {
> + ethernet0 =
> + };
> +
> + reg_gmac_3v3: gmac-3v3 {
> + compatible = "regulator-fixed";
> + pinctrl-names = "default";
> + pinctrl-0 = <_power_pin_nanopi>;
> + regulator-name = "gmac-3v3";
> + regulator-min-microvolt = <330>;
> + regulator-max-microvolt = <330>;
> + startup-delay-us = <10>;
> + enable-active-high;
> + gpio = < 3 6 GPIO_ACTIVE_HIGH>;
> + };
> +};
> +
> + {
> + status = "okay";
> +};
> +
> + {
> + status = "okay";
> +};
> +
> + {
> + gmac_power_pin_nanopi: gmac_power_pin@0 {
> + pins = "PD6";
> + function = "gpio_out";
> + };
> +};
You don't need that node, it's going to be done automatically with the
GPIO you defined earlier.
> +_mdio {
> + ext_rgmii_phy: ethernet-phy@1 {
> + compatible = "ethernet-phy-ieee802.3-c22";
> + reg = <7>;
That address doesn't match the unit-adress in the DT node name.
> + };
> +};
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_rgmii_pins>;
> + phy-supply = <_gmac_3v3>;
> + phy-handle = <_rgmii_phy>;
> + phy-mode = "rgmii";
Have you tested the network with the 5.9 kernel? There's been a lot of
DT broken due to a change in the PHY setup.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH] arm64: dts: allwinner: a64: OrangePi Win: Fix ethernet node
On Thu, Oct 22, 2020 at 08:58:39PM +0200, Jernej Skrabec wrote:
> RX/TX delay on OrangePi Win board is set on PHY. Reflect that in
> ethernet node.
>
> Fixes: 93d6a27cfcc0 ("arm64: dts: allwinner: a64: Orange Pi Win: Add Ethernet
> node")
> Signed-off-by: Jernej Skrabec
Queued as a fix for 5.10, thanks!
Maxime
signature.asc
Description: PGP signature
Re: [PATCH] arm64: dts: allwinner: a64: Pine64 Plus: Fix ethernet node
On Thu, Oct 22, 2020 at 11:13:01PM +0200, Jernej Skrabec wrote:
> According to board schematic, PHY provides both, RX and TX delays.
> However, according to "fix" Realtek provided for this board, only TX
> delay should be provided by PHY.
> Tests show that both variants work but TX only PHY delay works
> slightly better.
>
> Update ethernet node to reflect the fact that PHY provides TX delay.
>
> Fixes: 94f442886711 ("arm64: dts: allwinner: A64: Restore EMAC changes")
> Signed-off-by: Jernej Skrabec
Queued as a fix for 5.10, thanks!
Maxime
signature.asc
Description: PGP signature
Re: [PATCH 24/29] arm64: dts: allwinner: h6: Harmonize DWC USB3 DT nodes name
On Tue, Oct 20, 2020 at 02:59:54PM +0300, Serge Semin wrote: > In accordance with the DWC USB3 bindings the corresponding node > name is suppose to comply with the Generic USB HCD DT schema, which > requires the USB nodes to have the name acceptable by the regexp: > "^usb(@.*)?" . Make sure the "snps,dwc3"-compatible nodes are correctly > named. > > Signed-off-by: Serge Semin Queued for 5.11, thanks! Maxime signature.asc Description: PGP signature
Re: [PATCH] arm64: dts: allwinner: Pine H64: Enable both RGMII RX/TX delay
On Mon, Oct 19, 2020 at 06:34:49AM +, Corentin Labbe wrote:
> Since commit bbc4d71d6354 ("net: phy: realtek: fix rtl8211e rx/tx delay
> config"),
> the network is unusable on PineH64 model A.
>
> This is due to phy-mode incorrectly set to rgmii instead of rgmii-id.
>
> Fixes: 729e1ffcf47e ("arm64: allwinner: h6: add support for the Ethernet on
> Pine H64")
> Signed-off-by: Corentin Labbe
Queued as a fix for 5.10, thanks!
Maxime
signature.asc
Description: PGP signature
Re: [PATCH] arm64: dts: allwinner: beelink-gs1: Enable both RGMII RX/TX delay
On Sun, Oct 18, 2020 at 07:24:09PM +0200, Clément Péron wrote:
> Before the commit:
> net: phy: realtek: fix rtl8211e rx/tx delay config
>
> The software overwrite for RX/TX delays of the RTL8211e were not
> working properly and the Beelink GS1 had both RX/TX delay of RGMII
> interface set using pull-up on the TXDLY and RXDLY pins.
>
> Now that these delays are working properly they overwrite the HW
> config and set this to 'rgmii' meaning no delay on both RX/TX.
> This makes the ethernet of this board not working anymore.
>
> Set the phy-mode to 'rgmii-id' meaning RGMII with RX/TX delays
> in the device-tree to keep the correct configuration.
>
> Fixes: 089bee8dd119 ("arm64: dts: allwinner: h6: Introduce Beelink GS1 board")
> Signed-off-by: Clément Péron
Fixed the commit ID and queued as a fix for 5.10
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v3] spi: spi-sun6i: implement DMA-based transfer mode
On Thu, Oct 22, 2020 at 10:52:21AM +0300, Alexander Kochetkov wrote: > From: Alexander Kochetkov > > DMA-based transfer will be enabled if data length is larger than FIFO size > (64 bytes for A64). This greatly reduce number of interrupts for > transferring data. > > For smaller data size PIO mode will be used. In PIO mode whole buffer will > be loaded into FIFO. > > If driver failed to request DMA channels then it fallback for PIO mode. > > Tested on SOPINE (https://www.pine64.org/sopine/) > > Signed-off-by: Alexander Kochetkov Acked-by: Maxime Ripard Thanks! Maxime signature.asc Description: PGP signature
Re: Context expectations in ALSA
Hi Mark On Thu, Oct 22, 2020 at 02:50:53PM +0100, Mark Brown wrote: > On Thu, Oct 22, 2020 at 11:50:41AM +0200, Maxime Ripard wrote: > > > This is caused by the HDMI driver polling some status bit that reports > > that the infoframes have been properly sent, and calling usleep_range > > between each iteration[1], and that is done in our trigger callback that > > seems to be run with a spinlock taken and the interrupt disabled > > (snd_pcm_action_lock_irq) as part of snd_pcm_start_lock_irq. This is the > > entire stack trace: > > That doesn't sound like something I would expect you do be doing in the > trigger callback TBH - it feels like if this is something that could > block then the setup should have been done during parameter > configuration or something rather than in trigger. > > > It looks like the snd_soc_dai_link structure has a nonatomic flag that > > seems to be made to address more or less that issue, taking a mutex > > instead of a spinlock. However setting that flag results in another > > lockdep issue, since the dmaengine controller doing the DMA transfer > > would call snd_pcm_period_elapsed on completion, in a tasklet, this time > > taking a mutex in an atomic context which is just as bad as the initial > > issue. This is the stacktrace this time: > > Like Jaroslav says you could punt to a workqueue here. I'd be more > inclined to move the sleeping stuff out of the trigger operations but > that'd avoid the issue too. There are some drivers doing this already > IIRC. > > > So, I'm not really sure what I'm supposed to do here. The drivers > > involved don't appear to be doing anything extraordinary, but the issues > > lockdep report are definitely valid too. What are the expectations in > > terms of context from ALSA when running the callbacks, and how can we > > fix it? > > To me having something in the trigger that needs waiting for is the bit > that feels the most awkward fit here, trigger is supposed to run very > quickly. Indeed, other DRM devices seem to send the infoframes as part of hw_params, and it solves our issue there too. I'll send a patch Thanks for the suggestion! Maxime signature.asc Description: PGP signature
Re: Context expectations in ALSA
Hi Takashi, On Thu, Oct 22, 2020 at 03:20:49PM +0200, Takashi Iwai wrote: > On Thu, 22 Oct 2020 14:57:41 +0200, > Maxime Ripard wrote: > > > > On Thu, Oct 22, 2020 at 12:03:19PM +0200, Jaroslav Kysela wrote: > > > Dne 22. 10. 20 v 11:50 Maxime Ripard napsal(a): > > > > > > > So, I'm not really sure what I'm supposed to do here. The drivers > > > > involved don't appear to be doing anything extraordinary, but the issues > > > > lockdep report are definitely valid too. What are the expectations in > > > > terms of context from ALSA when running the callbacks, and how can we > > > > fix it? > > > > > > I think that you should set the non-atomic flag and wake up the workqueue > > > or > > > so from interrupt handler in this case. Call snd_pcm_period_elapsed() > > > from the > > > workqueue not the interrupt handler context. > > > > Yeah, that was my first guess too. However, the DMA driver uses some > > kind of generic helpers using a tasklet, so getting rid of it would take > > some work and would very likely not be eligible for stable. > > Who sets the nonatomic flag for vc4? I couldn't find the relevant > code in the latest upstream. Sorry if this wasn't clear enough, it's not there at the moment, ALSA takes a spinlock and lockdep complains that we're sleeping in an atomic context. I tried to add the nonatomic flag in my tree to see if it was fixing the issue, but ran into another lockdep complain now with ALSA taking a mutex in a tasklet. > Ideally dmaengine PCM helper should support the nonatomic mode, but > until then, the other side needs to drop the nonatomic flag, I > suppose. In this case, I'm not sure the blame is in the PCM helper but if there's any blame, I guess it's the virt-chan layer inside dmaengine (so for providers) that use a tasklet instead of something that allows sleeping Maxime signature.asc Description: PGP signature
Re: Context expectations in ALSA
On Thu, Oct 22, 2020 at 12:03:19PM +0200, Jaroslav Kysela wrote: > Dne 22. 10. 20 v 11:50 Maxime Ripard napsal(a): > > > So, I'm not really sure what I'm supposed to do here. The drivers > > involved don't appear to be doing anything extraordinary, but the issues > > lockdep report are definitely valid too. What are the expectations in > > terms of context from ALSA when running the callbacks, and how can we > > fix it? > > I think that you should set the non-atomic flag and wake up the workqueue or > so from interrupt handler in this case. Call snd_pcm_period_elapsed() from the > workqueue not the interrupt handler context. Yeah, that was my first guess too. However, the DMA driver uses some kind of generic helpers using a tasklet, so getting rid of it would take some work and would very likely not be eligible for stable. Maxime signature.asc Description: PGP signature
Context expectations in ALSA
Hi! We currently have an issue reported by lockdep on the RaspberryPi and its HDMI audio output where, at startup, we end up scheduling in atomic context. This is caused by the HDMI driver polling some status bit that reports that the infoframes have been properly sent, and calling usleep_range between each iteration[1], and that is done in our trigger callback that seems to be run with a spinlock taken and the interrupt disabled (snd_pcm_action_lock_irq) as part of snd_pcm_start_lock_irq. This is the entire stack trace: # aplay --channels=2 -D 'iec958:CARD=vc4hdmi0,DEV=0' /root/test-tone.wav Playing WAVE '/root/test-tone.wav' : Signed 16 bit Little Endian, Rate 44100 Hz, Stereo [ 14.732730] BUG: sleeping function called from invalid context at drivers/gpu/drm/vc4/vc4_hdmi.c:276 [ 14.742005] in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 140, name: aplay [ 14.749955] CPU: 0 PID: 140 Comm: aplay Not tainted 5.9.0-rc5-v7l+ #66 [ 14.756578] Hardware name: BCM2711 [ 14.760026] Backtrace: [ 14.762524] [] (dump_backtrace) from [] (show_stack+0x20/0x24) [ 14.770209] r7:c167c9e4 r6:600e0093 r5: r4:c167c9e4 [ 14.775960] [] (show_stack) from [] (dump_stack+0xb8/0xd8) [ 14.783297] [] (dump_stack) from [] (___might_sleep+0x138/0x17c) [ 14.791159] r10:0084 r9:f0979b00 r8:c10ffd50 r7:0010 r6: r5:0114 [ 14.799105] r4:c10fe000 r3:600e0093 [ 14.802736] [] (___might_sleep) from [] (__might_sleep+0x70/0xb0) [ 14.810680] r4:c0e8f5f4 [ 14.813256] [] (__might_sleep) from [] (vc4_hdmi_stop_packet+0x120/0x330) [ 14.821907] r6:ef21f280 r5:0003 r4:73e746f9 [ 14.826598] [] (vc4_hdmi_stop_packet) from [] (vc4_hdmi_write_infoframe+0x140/0x5d0) [ 14.836220] r9:f0979b00 r8:c10ffd50 r7:ef219900 r6:ef248840 r5: r4:ef21f280 [ 14.844084] [] (vc4_hdmi_write_infoframe) from [] (vc4_hdmi_set_audio_infoframe+0x5c/0x80) [ 14.854236] r10:efac7738 r9:ef3ea240 r8:0001 r7:ef219900 r6:ef248840 r5:ef21f040 [ 14.862180] r4:ef21f280 [ 14.864756] [] (vc4_hdmi_set_audio_infoframe) from [] (vc4_hdmi_audio_trigger+0x38/0x238) [ 14.874815] r4:0001 [ 14.877392] [] (vc4_hdmi_audio_trigger) from [] (snd_soc_pcm_dai_trigger+0x64/0xcc) [ 14.886923] r5:efac5a00 r4: [ 14.890555] [] (snd_soc_pcm_dai_trigger) from [] (soc_pcm_trigger+0x70/0xac) [ 14.899472] r9:ef3ea240 r8:c1133540 r7:0003 r6:ef219900 r5:ef219900 r4:0001 [ 14.907336] [] (soc_pcm_trigger) from [] (snd_pcm_do_start+0x3c/0x40) [ 14.915633] r5:c0c749dc r4: [ 14.919264] [] (snd_pcm_do_start) from [] (snd_pcm_action_single+0x48/0x88) [ 14.928094] [] (snd_pcm_action_single) from [] (snd_pcm_action+0x6c/0x78) [ 14.936746] r7:0003 r6:c0c749dc r5: r4:ef219900 [ 14.942493] [] (snd_pcm_action) from [] (snd_pcm_action_lock_irq+0x38/0x50) [ 14.951320] r7:0030 r6:0003 r5:c0c749dc r4:ef219900 [ 14.957069] [] (snd_pcm_action_lock_irq) from [] (snd_pcm_ioctl+0x930/0x14e4) [ 14.966073] r7:0030 r6: r5:0085ee60 r4:ef219900 [ 14.971823] [] (snd_pcm_ioctl) from [] (sys_ioctl+0xe4/0x9e4) [ 14.979420] r10:efac7738 r9:0004 r8:c1133540 r7:0085ee60 r6:5624 r5:c1133540 [ 14.987364] r4:4142 [ 14.989940] [] (sys_ioctl) from [] (ret_fast_syscall+0x0/0x28) [ 14.997621] Exception stack(0xc10fffa8 to 0xc100) [ 15.002749] ffa0: 00869260 b6fa8000 0004 4142 0085ee60 0001 [ 15.011050] ffc0: 00869260 b6fa8000 5624 0036 1589 1589 [ 15.019350] ffe0: b6fa8504 bedb59fc b6f2ff68 b6da515c [ 15.024479] r10:0036 r9:c10fe000 r8:c0200204 r7:0036 r6:5624 r5:b6fa8000 [ 15.032423] r4:00869260 We could switch to a udelay instead, but the idea of busy-waiting for up to a 100ms while having the interrupt disabled doesn't sound ideal either. It looks like the snd_soc_dai_link structure has a nonatomic flag that seems to be made to address more or less that issue, taking a mutex instead of a spinlock. However setting that flag results in another lockdep issue, since the dmaengine controller doing the DMA transfer would call snd_pcm_period_elapsed on completion, in a tasklet, this time taking a mutex in an atomic context which is just as bad as the initial issue. This is the stacktrace this time: # aplay --channels=2 -D 'iec958:CARD=vc4hdmi0,DEV=0' /root/test-tone.wav Playing WAVE '/root/test-tone.wav' : Signed 16 bit Little Endian, Rate 44100 Hz, Stereo [ 43.078900] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:281 [ 43.087485] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 0, name: swapper/0 [ 43.095452] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.9.0-rc5-v7l+ #67 [ 43.102252] Hardware name: BCM2711 [ 43.105701] Backtrace: [ 43.108199] [] (dump_backtrace) from [] (show_stack+0x20/0x24) [ 43.115884] r7:c167c9e4 r6:600e0113 r5: r4:c167c9e4 [
Re: [PATCH] media: cedrus: h264: Fix check for presence of scaling matrix
On Wed, Oct 21, 2020 at 10:33:25PM +0200, Jernej Skrabec wrote:
> If scaling matrix control is present, VPU should not use default matrix.
> Fix that.
>
> Fixes: b3a23db0e2f8 ("media: cedrus: Use H264_SCALING_MATRIX only when
> required")
> Signed-off-by: Jernej Skrabec
Acked-by: Maxime Ripard
Thanks!
Maxime
signature.asc
Description: PGP signature
Re: [PATCH] spi: spi-sun6i: implement DMA-based transfer mode
Hi,
On Mon, Oct 19, 2020 at 04:17:18PM +0300, Alexander Kochetkov wrote:
> >> +static int sun6i_spi_prepare_dma(struct sun6i_spi *sspi,
> >> + struct spi_transfer *tfr)
> >> +{
> >> + struct dma_async_tx_descriptor *rxdesc, *txdesc;
> >> + struct spi_master *master = sspi->master;
> >> +
> >> + rxdesc = NULL;
> >> + if (tfr->rx_buf) {
> >> + struct dma_slave_config rxconf = {
> >> + .direction = DMA_DEV_TO_MEM,
> >> + .src_addr = sspi->dma_addr_rx,
> >> + .src_addr_width = 1,
> >> + .src_maxburst = 1,
> >> + };
> >
> > That doesn't really look optimal, the controller seems to be able to
> > read / write 32 bits at a time from its FIFO and we probably can
> > increase the burst length too?
>
>
> I had doubts if it would work. I didn’t know will DMA work for
> transfers with lengths not aligned to 32 bits. For example, if we init
> DMA with src_addr_width = 1 and .src_maxburst = 8 will DMA work for
> transfer with length 11?
Bursts are usually defined by how many transfers the controller is
allowed to do at once, so it shouldn't cause any harm if the length
isn't aligned, it will just do less than the maximum number allowed.
Whether or not the hardware agrees to that definition is something else
though, but from experience it should
> I expect that DMA fill FIFO with 16 bytes and
> SPI transfer only 11 bytes and 5 bytes will leave in TX fifo. I did
> the test and there is no additional data left in the fifo buffer. Also
> at reception the is no memory overwrites.
>
> I made test with src_addr_width = 4, src_maxburst = 1 and transfer
> length 3. Looks like in that case DMA doesn’t issue 4 bytes transfer.
>
> For test with src_addr_width = 4, src_maxburst = 8 I had to adjust
> RF_RDY_TRIG_LEVEL_BITS and TF_ERQ_TRIG_LEVEL_BITS of FIFO_CTL_REG to
> half of FIFO (32 bytes). With the config DMA will transfer burst of
> half of FIFO length during transfer and remaining bytes at the end of
> transfer.
Yeah, that might need some tuning. With the width, I guess we should pay
attention to the order the bytes are sent in, but it can be done later.
> >>
> >> @@ -343,7 +436,8 @@ static irqreturn_t sun6i_spi_handler(int irq, void
> >> *dev_id)
> >>/* Transfer complete */
> >>if (status & SUN6I_INT_CTL_TC) {
> >>sun6i_spi_write(sspi, SUN6I_INT_STA_REG, SUN6I_INT_CTL_TC);
> >> - sun6i_spi_drain_fifo(sspi);
> >> + if (!sspi->use_dma)
> >> + sun6i_spi_drain_fifo(sspi);
> >
> > Is it causing any issue? The FIFO will be drained only if there's
> > something remaining in the FIFO, which shouldn't happen with DMA?
> >
>
> No. It’s for make code patch explicit.
> Remove the change?
Yes, that also simplifies the driver since we don't have to rely on the
boolean in the main structure anymore
Maxime
signature.asc
Description: PGP signature
Re: [PATCH] spi: spi-sun6i: implement DMA-based transfer mode
On Tue, Oct 20, 2020 at 11:52:34AM +0800, Chen-Yu Tsai wrote: > On Tue, Oct 20, 2020 at 1:43 AM Alexander Kochetkov > wrote: > > > > > > > > > 19 окт. 2020 г., в 11:21, Maxime Ripard написал(а): > > > > > > Hi! > > > > > > On Thu, Oct 15, 2020 at 06:47:40PM +0300, Alexander Kochetkov wrote: > > >> DMA-based transfer will be enabled if data length is larger than FIFO > > >> size > > >> (64 bytes for A64). This greatly reduce number of interrupts for > > >> transferring data. > > >> > > >> For smaller data size PIO mode will be used. In PIO mode whole buffer > > >> will > > >> be loaded into FIFO. > > >> > > >> If driver failed to request DMA channels then it fallback for PIO mode. > > >> > > >> Tested on SOPINE (https://www.pine64.org/sopine/) > > >> > > >> Signed-off-by: Alexander Kochetkov > > > > > > Thanks for working on this, it's been a bit overdue > > > > Hi, Maxime! > > > > We did custom A64 based computation module for our product. > > Do you mean that A64 is obsolete or EOL product? > > If so, can you recommend active replacement for A64 from Allwinner same > > price? > > I believe what Maxime meant was that DMA transfer for SPI is a long > sought-after feature, but no one had finished it. Yeah, that's what I meant :) Maxime signature.asc Description: PGP signature
Re: [PATCH v2] spi: spi-sun6i: enable autosuspend feature
On Mon, Oct 19, 2020 at 06:03:43PM +0300, Alexander Kochetkov wrote: > From: Alexander Kochetkov > > If SPI is used for periodic polling any sensor, significant delays > sometimes appear. Switching on module clocks during resume lead to delays. > Enabling autosuspend mode causes the controller to not suspend between > SPI transfers and the delays disappear. > > The commit also remove unnecessary call to pm_runtime_idle() used > to explicit put device to suspended state. Without pm_runtime_idle() PM > core will put device in the suspended state just after probe() returns. > > Signed-off-by: Alexander Kochetkov Acked-by: Maxime Ripard Thanks! Maxime signature.asc Description: PGP signature
Re: [PATCH v2] arm64: dts: allwinner: beelink-gs1: Update LED power node
On Sun, Oct 18, 2020 at 07:25:10PM +0200, Clément Péron wrote:
> HI Maxime,
>
> On Mon, 12 Oct 2020 at 13:22, Maxime Ripard wrote:
> >
> > Hi!
> >
> > On Sun, Oct 11, 2020 at 11:22:37PM +0200, Clément Péron wrote:
> > > Beelink GS1 LED trigger a warning when running dtbs_check.
> > >
> > > Update the node with a valid pattern property.
> > >
> > > Also add the function and the color of the LED and drop the
> > > label which is deprecated.
> > >
> > > Signed-off-by: Clément Péron
> > > ---
> > > arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts | 6 --
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > > b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > > index 3f7ceeb1a767..a364cb4e5b3f 100644
> > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > > @@ -7,6 +7,7 @@
> > > #include "sun50i-h6-cpu-opp.dtsi"
> > >
> > > #include
> > > +#include
> > >
> > > / {
> > > model = "Beelink GS1";
> > > @@ -43,8 +44,9 @@ ext_osc32k: ext_osc32k_clk {
> > > leds {
> > > compatible = "gpio-leds";
> > >
> > > - power {
> > > - label = "beelink:white:power";
> > > + led-0 {
> > > + function = LED_FUNCTION_POWER;
> > > + color = ;
> > > gpios = <_pio 0 4 GPIO_ACTIVE_HIGH>; /* PL4 */
> > > default-state = "on";
> > > };
> >
> > Doesn't that also change the sysfs file that LED is exposed to the
> > userspace with?
>
> Indeed the previous led sysfs:
> /sys/class/leds/beelink:white:power/
> is now
> /sys/class/leds/white:power/
>
> Do you want me to keep the label property to avoid this sysfs change ?
I don't know, the documentation seems to indicate that we should use one
or the other, but I'm not sure if both makes sense or not. I guess we
should ask Pavel or Rob?
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v6 02/14] ASoC: sun4i-i2s: Change set_chan_cfg() params
Hi Samuel,
On Mon, Oct 12, 2020 at 08:15:30PM -0500, Samuel Holland wrote:
> On 10/12/20 7:15 AM, Maxime Ripard wrote:
> > Hi,
> >
> > On Mon, Oct 05, 2020 at 03:23:12PM +0200, Clément Péron wrote:
> >> On Mon, 5 Oct 2020 at 14:13, Maxime Ripard wrote:
> >>>
> >>> On Sat, Oct 03, 2020 at 04:19:38PM +0200, Clément Péron wrote:
> >>>> As slots and slot_width can be set manually using set_tdm().
> >>>> These values are then kept in sun4i_i2s struct.
> >>>> So we need to check if these values are setted or not
> >>>> in the struct.
> >>>>
> >>>> Avoid to check for this logic in set_chan_cfg(). This will
> >>>> duplicate the same check instead pass the required values
> >>>> as params to set_chan_cfg().
> >>>>
> >>>> This will also avoid a bug when we will enable 20/24bits support,
> >>>> i2s->slot_width is not actually used in the lrck_period computation.
> >>>>
> >>>> Suggested-by: Samuel Holland
> >>>> Signed-off-by: Clément Péron
> >>>> ---
> >>>> sound/soc/sunxi/sun4i-i2s.c | 36 ++--
> >>>> 1 file changed, 14 insertions(+), 22 deletions(-)
> >>>>
> >>>> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> >>>> index c5ccd423e6d3..1f577dbc20a6 100644
> >>>> --- a/sound/soc/sunxi/sun4i-i2s.c
> >>>> +++ b/sound/soc/sunxi/sun4i-i2s.c
> >>>> @@ -177,8 +177,9 @@ struct sun4i_i2s_quirks {
> >>>> unsigned long (*get_bclk_parent_rate)(const struct sun4i_i2s *);
> >>>> s8 (*get_sr)(const struct sun4i_i2s *, int);
> >>>> s8 (*get_wss)(const struct sun4i_i2s *, int);
> >>>> - int (*set_chan_cfg)(const struct sun4i_i2s *,
> >>>> - const struct snd_pcm_hw_params *);
> >>>> + int (*set_chan_cfg)(const struct sun4i_i2s *i2s,
> >>>> + unsigned int channels, unsigned int slots,
> >>>> + unsigned int slot_width);
> >>>> int (*set_fmt)(const struct sun4i_i2s *, unsigned int);
> >>>> };
> >>>>
> >>>> @@ -414,10 +415,9 @@ static s8 sun8i_i2s_get_sr_wss(const struct
> >>>> sun4i_i2s *i2s, int width)
> >>>> }
> >>>>
> >>>> static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> >>>> - const struct snd_pcm_hw_params *params)
> >>>> + unsigned int channels, unsigned int
> >>>> slots,
> >>>> + unsigned int slot_width)
> >>>> {
> >>>> - unsigned int channels = params_channels(params);
> >>>> -
> >>>> /* Map the channels for playback and capture */
> >>>> regmap_write(i2s->regmap, SUN4I_I2S_TX_CHAN_MAP_REG, 0x76543210);
> >>>> regmap_write(i2s->regmap, SUN4I_I2S_RX_CHAN_MAP_REG, 0x3210);
> >>>> @@ -434,15 +434,11 @@ static int sun4i_i2s_set_chan_cfg(const struct
> >>>> sun4i_i2s *i2s,
> >>>> }
> >>>>
> >>>> static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> >>>> - const struct snd_pcm_hw_params *params)
> >>>> + unsigned int channels, unsigned int
> >>>> slots,
> >>>> + unsigned int slot_width)
> >>>> {
> >>>> - unsigned int channels = params_channels(params);
> >>>> - unsigned int slots = channels;
> >>>> unsigned int lrck_period;
> >>>>
> >>>> - if (i2s->slots)
> >>>> - slots = i2s->slots;
> >>>> -
> >>>> /* Map the channels for playback and capture */
> >>>> regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_MAP_REG, 0x76543210);
> >>>> regmap_write(i2s->regmap, SUN8I_I2S_RX_CHAN_MAP_REG, 0x76543210);
> >>>> @@ -467,11 +463,11 @@ static int sun8i_i2s_set_chan_cfg(const struct
> >>>> sun4i_i2s *i2s,
> >>>> case SND_SOC_DAIFMT_DSP_B:
> >>>> case SND_SOC_DAIFMT_LEFT_J:
> >>>>
Re: [PATCH] ARM: dts: sun8i: h2+: Enable optional SPI flash on Orange Pi Zero board
On Mon, Oct 12, 2020 at 07:03:25PM +0200, Michal Suchánek wrote: > > > > > > > > > Also the boards that do not have the flsh are either broken or > > > > > obsolete. > > > > > > > > Making general statements without arguments doesn't really make it true > > > > though. Plenty of boards to have flash and are neither broken nor > > > > obsolete. > > > > > > Cannot parse this. > > > > "Plenty of boards do not have flash and are neither broken nor obsolete" > The product description of Orange Pi Zero clearly states there is a > flash memory: http://www.orangepi.org/orangepizero/ > > When you order an Orange Pi Zero it comes with a flash memory. That is > not what the device tree describes. The device tree is supposed to > descrbe the hardware. If it does not it is broken. > > If you have a board without a flash memory I do not know what it is but > it is clearly not an Orange Pi Zero because it comes with one. If you're buying it today, yes. If you take a random Orange Pi Zero that has been sold at any point in time, you cannot make that statement. > > > > > > > > > So most of the time enabling the flash chip is the right thing. > > > > > > > > > > Or do we need two DTBs like sun8i-h2-plus-orangepi-zero.dts and > > > > > sun8i-h2-plus-orangepi-zero-no-spi-nor.dts > > > > > > > > No, you need sun8i-h2-plus-orangepi-zero plus an overlay for the > > > > SPI-NOR. > > > > > > The flash is part of the board. > > > > Not always though. > No, it always comes with one. You must be speaking of a different board > then. > > > > > There is no need for an overlay. > > > > Overlays are here to deal with the "not always though" situation... > > There are no overlays in the kernel. Please show me tho code in the > kernel for handling overlays. You're the one that mentioned the kernel here, but here you are: https://elixir.bootlin.com/linux/v5.9.1/source/include/linux/of.h#L1455 And a driver using it: https://elixir.bootlin.com/linux/v5.9.1/source/drivers/gpu/drm/rcar-du/rcar_du_of.c#L44 > > > And overlays don't exist. > > > > If you want to believe that, please go ahead. > > > > But there's support for it in libfdt, and you can either apply them > > directly through the U-Boot command line, or bundle them in a FIT image. > > And as you state the user ususally does not know which version of the Pi > they have. How are they supposed to know that they should apply an > overlay through u-boot commandline (if they even get to see one) Documentation? > bundle them in a FIT image (if they are even using a FIT image). That would be the distro's job, not the user's. > I am doing neither. I boot a standard distribution kernel from EFI > grub. > > I understand that it would be nice to support two almost identical > boards with a single device tree. However, if an error about missing > flash memory is not acceptable, and the kernel does not support > enabling the flash memory dynamically we need two device trees then. You keep moving the goalposts, but U-boot is perfectly able to apply an overlay automatically at boot without the user's intervention. We're already making it select various DTs for the pine64 and pine64+ to have a single image for both, or for the pinephone variants. Maxime signature.asc Description: PGP signature
Re: [PATCH 1/1] clocksource: sun4i: Save and restore timer registers before and after sleeping
Hi!
On Sat, Oct 10, 2020 at 06:46:03PM +0800, wuyan wrote:
> Signed-off-by: wuyan
A commit log would be welcome here. Also, the last time you contributed
you used the name Martin Wu in your Signed-off-by, it would be nice to
be consistent there.
> Change-Id: I7edbc00fd0968d0301757f5a75dbd6f53d6a7cd7
This should be removed
> ---
> drivers/clocksource/timer-sun4i.c | 45 +--
> 1 file changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clocksource/timer-sun4i.c
> b/drivers/clocksource/timer-sun4i.c
> index 0ba8155b8287..49fb6b90ec15 100644
> --- a/drivers/clocksource/timer-sun4i.c
> +++ b/drivers/clocksource/timer-sun4i.c
> @@ -29,6 +29,7 @@
> #define TIMER_IRQ_EN_REG 0x00
> #define TIMER_IRQ_EN(val)BIT(val)
> #define TIMER_IRQ_ST_REG 0x04
> +#define TIMER_IRQ_CLEAR(val) BIT(val)
> #define TIMER_CTL_REG(val) (0x10 * val + 0x10)
> #define TIMER_CTL_ENABLE BIT(0)
> #define TIMER_CTL_RELOAD BIT(1)
> @@ -41,6 +42,19 @@
>
> #define TIMER_SYNC_TICKS 3
>
> +/* Registers which needs to be saved and restored before and after sleeping
> */
> +static u32 regs_offset[] = {
It would be better to have a prefix (like sun4i_timer to be consistent)
there so that we know it's less confusing and we know it's not some
generic thing.
> + TIMER_IRQ_EN_REG,
> + TIMER_IRQ_ST_REG,
Why do you need to save the interrupt status register?
> + TIMER_CTL_REG(0),
> + TIMER_INTVAL_REG(0),
> + TIMER_CNTVAL_REG(0),
> + TIMER_CTL_REG(1),
> + TIMER_INTVAL_REG(1),
> + TIMER_CNTVAL_REG(1),
> +};
> +static u32 regs_backup[ARRAY_SIZE(regs_offset)];
We should store this one in the timer_of struct so that we don't have
any issue if there's two timers at some point.
> /*
> * When we disable a timer, we need to wait at least for 2 cycles of
> * the timer source clock. We will use for that the clocksource timer
> @@ -82,10 +96,37 @@ static void sun4i_clkevt_time_start(void __iomem *base,
> u8 timer,
> base + TIMER_CTL_REG(timer));
> }
>
> +static inline void save_regs(void __iomem *base)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(regs_offset); i++)
> + regs_backup[i] = readl(base + regs_offset[i]);
> +}
> +
> +static inline void restore_regs(void __iomem *base)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(regs_offset); i++)
> + writel(regs_backup[i], base + regs_offset[i]);
> +}
> +
Same thing here, using the prefix would be nice for those two functions
name.
> static int sun4i_clkevt_shutdown(struct clock_event_device *evt)
> {
> struct timer_of *to = to_timer_of(evt);
>
> + save_regs(timer_of_base(to));
> + sun4i_clkevt_time_stop(timer_of_base(to), 0);
> +
> + return 0;
> +}
> +
> +static int sun4i_tick_resume(struct clock_event_device *evt)
> +{
> + struct timer_of *to = to_timer_of(evt);
> +
> + restore_regs(timer_of_base(to));
> sun4i_clkevt_time_stop(timer_of_base(to), 0);
>
> return 0;
> @@ -126,7 +167,7 @@ static int sun4i_clkevt_next_event(unsigned long evt,
>
> static void sun4i_timer_clear_interrupt(void __iomem *base)
> {
> - writel(TIMER_IRQ_EN(0), base + TIMER_IRQ_ST_REG);
> + writel(TIMER_IRQ_CLEAR(0), base + TIMER_IRQ_ST_REG);
> }
This is mostly a cosmetic change right? Either way, it should be in a
separate patch.
Thanks!
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v2 15/17] ASoC: sun8i-codec: Generalize AIF clock control
On Wed, Oct 14, 2020 at 01:19:39AM -0500, Samuel Holland wrote: > The AIF clock control register has the same layout for all three AIFs. > The only difference between them is that AIF3 is missing some fields. We > can reuse the same register field definitions for all three registers, > and use the DAI ID to select the correct register address. > > Signed-off-by: Samuel Holland Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH v2 12/17] ASoC: sun8i-codec: Protect the clock rate while streams are open
On Wed, Oct 14, 2020 at 01:19:36AM -0500, Samuel Holland wrote: > The codec's clock input is shared among all AIFs, and shared with other > audio-related hardware in the SoC, including I2S and SPDIF controllers. > To ensure sample rates selected by userspace or by codec2codec DAI links > are maintained, the clock rate must be protected while it is in use. > > Signed-off-by: Samuel Holland > --- > sound/soc/sunxi/Kconfig | 1 + > sound/soc/sunxi/sun8i-codec.c | 29 +++-- > 2 files changed, 28 insertions(+), 2 deletions(-) > > diff --git a/sound/soc/sunxi/Kconfig b/sound/soc/sunxi/Kconfig > index 9cd7009cb570..69b9d8515335 100644 > --- a/sound/soc/sunxi/Kconfig > +++ b/sound/soc/sunxi/Kconfig > @@ -9,16 +9,17 @@ config SND_SUN4I_CODEC > help > Select Y or M to add support for the Codec embedded in the Allwinner > A10 and affiliated SoCs. > > config SND_SUN8I_CODEC > tristate "Allwinner SUN8I audio codec" > depends on OF > depends on MACH_SUN8I || (ARM64 && ARCH_SUNXI) || COMPILE_TEST > + select COMMON_CLK Wouldn't a depends on make more sense here? It's kind of weird to pull it from a driver when the platform that would run it has no CCF support. With this changed, Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH v2 10/17] ASoC: sun8i-codec: Automatically set the system sample rate
On Wed, Oct 14, 2020 at 01:19:34AM -0500, Samuel Holland wrote: > The sun8i codec has three clock/sample rate domains: > - The AIF1 domain, with a sample rate equal to AIF1 LRCK > - The AIF2 domain, with a sample rate equal to AIF2 LRCK > - The SYSCLK domain, containing the ADC, DAC, and effects (AGC/DRC), >with a sample rate given by a divisor from SYSCLK. The divisor is >controlled by the AIF1_FS or AIF2_FS field in SYS_SR_CTRL, depending >on if SYSCLK's source is AIF1CLK or AIF2CLK, respectively. The exact >sample rate depends on if SYSCLK is running at 22.6 MHz or 24.6 MHz. > > When an AIF (currently only AIF1) is active, the ADC and DAC should run > at that sample rate to avoid artifacting. Sample rate conversion is only > available when multiple AIFs are active and are routed to each other; > this means the sample rate conversion hardware usually cannot be used. > > Only attach the event hook to the channel 0 AIF widgets, since we only > need one event when a DAI stream starts or stops. Channel 0 is always > brought up with a DAI stream, regardless of the number of channels in > the stream. > > The ADC and DAC (along with their effects blocks) can be used even if > no AIFs are in use. In that case, we should select an appropriate sample > rate divisor, instead of keeping the last-used AIF sample rate. > 44.1/48 kHz was chosen to balance audio quality and power consumption. > > Since the sample rate is tied to active AIF paths, disabling pmdown_time > allows switching to the optimal sample rate immediately, instead of > after a 5 second delay. > > Signed-off-by: Samuel Holland Acked-by: Maxime Ripard Thanks! Maxime signature.asc Description: PGP signature
Re: [PATCH v2 03/17] ASoC: sun8i-codec: Enable all supported clock inversions
On Wed, Oct 14, 2020 at 01:19:27AM -0500, Samuel Holland wrote: > When using the I2S, LEFT_J, or RIGHT_J format, the hardware supports > independent BCLK and LRCK inversion control. When using DSP_A or DSP_B, > LRCK inversion is not supported. The register bit is repurposed to > select between DSP_A and DSP_B. Extend the driver to support this. > > Signed-off-by: Samuel Holland Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH] spi: spi-sun6i: implement DMA-based transfer mode
Hi!
On Thu, Oct 15, 2020 at 06:47:40PM +0300, Alexander Kochetkov wrote:
> DMA-based transfer will be enabled if data length is larger than FIFO size
> (64 bytes for A64). This greatly reduce number of interrupts for
> transferring data.
>
> For smaller data size PIO mode will be used. In PIO mode whole buffer will
> be loaded into FIFO.
>
> If driver failed to request DMA channels then it fallback for PIO mode.
>
> Tested on SOPINE (https://www.pine64.org/sopine/)
>
> Signed-off-by: Alexander Kochetkov
Thanks for working on this, it's been a bit overdue
> ---
> drivers/spi/spi-sun6i.c | 171 +---
> 1 file changed, 159 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
> index 19238e1b76b4..38e5b8af7da6 100644
> --- a/drivers/spi/spi-sun6i.c
> +++ b/drivers/spi/spi-sun6i.c
> @@ -18,6 +18,7 @@
> #include
> #include
> #include
> +#include
>
> #include
>
> @@ -52,10 +53,12 @@
>
> #define SUN6I_FIFO_CTL_REG 0x18
> #define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_MASK0xff
> +#define SUN6I_FIFO_CTL_RF_DRQ_EN BIT(8)
> #define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS0
> #define SUN6I_FIFO_CTL_RF_RSTBIT(15)
> #define SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_MASK0xff
> #define SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS16
> +#define SUN6I_FIFO_CTL_TF_DRQ_EN BIT(24)
> #define SUN6I_FIFO_CTL_TF_RSTBIT(31)
>
> #define SUN6I_FIFO_STA_REG 0x1c
> @@ -83,6 +86,8 @@
> struct sun6i_spi {
> struct spi_master *master;
> void __iomem*base_addr;
> + dma_addr_t dma_addr_rx;
> + dma_addr_t dma_addr_tx;
> struct clk *hclk;
> struct clk *mclk;
> struct reset_control*rstc;
> @@ -92,6 +97,7 @@ struct sun6i_spi {
> const u8*tx_buf;
> u8 *rx_buf;
> int len;
> + booluse_dma;
> unsigned long fifo_depth;
> };
>
> @@ -182,6 +188,66 @@ static size_t sun6i_spi_max_transfer_size(struct
> spi_device *spi)
> return SUN6I_MAX_XFER_SIZE - 1;
> }
>
> +static int sun6i_spi_prepare_dma(struct sun6i_spi *sspi,
> + struct spi_transfer *tfr)
> +{
> + struct dma_async_tx_descriptor *rxdesc, *txdesc;
> + struct spi_master *master = sspi->master;
> +
> + rxdesc = NULL;
> + if (tfr->rx_buf) {
> + struct dma_slave_config rxconf = {
> + .direction = DMA_DEV_TO_MEM,
> + .src_addr = sspi->dma_addr_rx,
> + .src_addr_width = 1,
> + .src_maxburst = 1,
> + };
That doesn't really look optimal, the controller seems to be able to
read / write 32 bits at a time from its FIFO and we probably can
increase the burst length too?
> +
> + dmaengine_slave_config(master->dma_rx, );
> +
> + rxdesc = dmaengine_prep_slave_sg(
> + master->dma_rx,
> + tfr->rx_sg.sgl, tfr->rx_sg.nents,
> + DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT);
checkpatch --strict complains about this one (your line shouldn't end
with a parenthesis).
> + if (!rxdesc)
> + return -EINVAL;
> + }
> +
> + txdesc = NULL;
> + if (tfr->tx_buf) {
> + struct dma_slave_config txconf = {
> + .direction = DMA_MEM_TO_DEV,
> + .dst_addr = sspi->dma_addr_tx,
> + .dst_addr_width = 1,
> + .dst_maxburst = 1,
> + };
> +
> + dmaengine_slave_config(master->dma_tx, );
> +
> + txdesc = dmaengine_prep_slave_sg(
> + master->dma_tx,
> + tfr->tx_sg.sgl, tfr->tx_sg.nents,
> + DMA_MEM_TO_DEV, DMA_PREP_INTERRUPT);
> + if (!txdesc) {
> + if (rxdesc)
> + dmaengine_terminate_sync(master->dma_rx);
> + return -EINVAL;
> + }
> + }
> +
> + if (tfr->rx_buf) {
> + dmaengine_submit(rxdesc);
> + dma_async_issue_pending(master->dma_rx);
> + }
> +
> + if (tfr->tx_buf) {
> + dmaengine_submit(txdesc);
> + dma_async_issue_pending(master->dma_tx);
> + }
> +
> + return 0;
> +}
> +
> static int sun6i_spi_transfer_one(struct spi_master *master,
> struct spi_device *spi,
> struct spi_transfer *tfr)
> @@ -201,6 +267,8 @@ static int sun6i_spi_transfer_one(struct spi_master
> *master,
> sspi->tx_buf = tfr->tx_buf;
> sspi->rx_buf = tfr->rx_buf;
> sspi->len = tfr->len;
> +
Re: [PATCH] [v2] rtc: sun6i: Fix memleak in sun6i_rtc_clk_init
Hi,
On Sun, Oct 18, 2020 at 03:28:10PM +0800, Dinghao Liu wrote:
> When clk_hw_register_fixed_rate_with_accuracy() fails,
> clk_data should be freed. It's the same for the subsequent
> two error paths, but we should also unregister the already
> registered clocks in them.
>
> Signed-off-by: Dinghao Liu
> ---
>
> Changelog:
>
> v2: - Unregister the already registered clocks on failure.
> ---
> drivers/rtc/rtc-sun6i.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> index e2b8b150bcb4..6de0d3ad736a 100644
> --- a/drivers/rtc/rtc-sun6i.c
> +++ b/drivers/rtc/rtc-sun6i.c
> @@ -272,7 +272,7 @@ static void __init sun6i_rtc_clk_init(struct device_node
> *node,
> 3);
> if (IS_ERR(rtc->int_osc)) {
> pr_crit("Couldn't register the internal oscillator\n");
> - return;
> + goto err;
> }
>
> parents[0] = clk_hw_get_name(rtc->int_osc);
> @@ -290,7 +290,8 @@ static void __init sun6i_rtc_clk_init(struct device_node
> *node,
> rtc->losc = clk_register(NULL, >hw);
> if (IS_ERR(rtc->losc)) {
> pr_crit("Couldn't register the LOSC clock\n");
> - return;
> + clk_hw_unregister_fixed_rate(rtc->int_osc);
> + goto err;
> }
The point of having labels for the error sequence is to avoid to
duplicate the error handling code in each and every error code path.
You should add another label for the fixed rate clock unregistration
Maxime
signature.asc
Description: PGP signature
Re: [PATCH] spi: spi-sun6i: enable autosuspend feature
Hi, On Fri, Oct 16, 2020 at 11:38:26AM +0300, Alexander Kochetkov wrote: > If SPI is used for periodic polling any sensor, significant delays > sometimes appear. Switching on module clocks during resume lead to delays. > Enabling autosuspend mode causes the controller to not suspend between > SPI transfers and the delays disappear. > > Signed-off-by: Alexander Kochetkov > --- > drivers/spi/spi-sun6i.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c > index 38e5b8af7da6..4cc0280e934c 100644 > --- a/drivers/spi/spi-sun6i.c > +++ b/drivers/spi/spi-sun6i.c > @@ -22,6 +22,8 @@ > > #include > > +#define SUN6I_AUTOSUSPEND_TIMEOUT2000 > + > #define SUN6I_FIFO_DEPTH 128 > #define SUN8I_FIFO_DEPTH 64 > > @@ -639,9 +641,10 @@ static int sun6i_spi_probe(struct platform_device *pdev) > goto err_free_dma_rx; > } > > + pm_runtime_set_autosuspend_delay(>dev, SUN6I_AUTOSUSPEND_TIMEOUT); > + pm_runtime_use_autosuspend(>dev); > pm_runtime_set_active(>dev); > pm_runtime_enable(>dev); > - pm_runtime_idle(>dev); You should also mention why pm_runtime_idle isn't useful anymore in your commit log. Thanks! Maxime signature.asc Description: PGP signature
Re: [PATCH 20/20] arch: dts: Fix DWC USB3 DT nodes name
On Thu, Oct 15, 2020 at 01:15:37PM +0300, Felipe Balbi wrote: > Serge Semin writes: > > > On Wed, Oct 14, 2020 at 05:09:37PM +0300, Felipe Balbi wrote: > >> > >> Hi Serge, > >> > >> Serge Semin writes: > >> > In accordance with the DWC USB3 bindings the corresponding node name is > >> > suppose to comply with Generic USB HCD DT schema, which requires the USB > >> > > > >> DWC3 is not a simple HDC, though. > > > > Yeah, strictly speaking it is equipped with a lot of vendor-specific stuff, > > which are tuned by the DWC USB3 driver in the kernel. But after that the > > controller is registered as xhci-hcd device so it's serviced by the xHCI > > driver, > > in Dual-role or host-only builds, that's correct. We can also have > peripheral-only builds (both SW or HW versions) which means xhci isn't > even in the picture. It doesn't really matter though, or at least it does for what the new name might be, but the old one currently used is still pretty bad. The DT spec says that the node name is the class of the device. "usb" as the HCD binding mandates is one, but the current nodes currently have completely different names from one DT to another - which is already an issue - and most of them have dwc3 or some variant of it, which doesn't really qualify for a class name. Maxime signature.asc Description: PGP signature
Re: [PATCH v2] arm64: dts: allwinner: h6: add eMMC voltage property for Beelink GS1
On Tue, Oct 13, 2020 at 11:27:33PM +0200, Jernej Škrabec wrote:
> Dne petek, 09. oktober 2020 ob 09:36:51 CEST je Maxime Ripard napisal(a):
> > On Thu, Oct 08, 2020 at 10:00:06PM +0200, Clément Péron wrote:
> > > Hi Maxime,
> > >
> > > Adding linux-sunxi and Jernej Skrabec to this discussion.
> > >
> > > On Thu, 8 Oct 2020 at 17:10, Maxime Ripard wrote:
> > > >
> > > > Hi Clément,
> > > >
> > > > On Mon, Oct 05, 2020 at 08:47:19PM +0200, Clément Péron wrote:
> > > > > On Mon, 5 Oct 2020 at 11:21, Maxime Ripard wrote:
> > > > > >
> > > > > > Hi Clément,
> > > > > >
> > > > > > On Sat, Oct 03, 2020 at 11:20:01AM +0200, Clément Péron wrote:
> > > > > > > Sunxi MMC driver can't distinguish at runtime what's the I/O
> voltage
> > > > > > > for HS200 mode.
> > > > > >
> > > > > > Unfortunately, that's not true (or at least, that's not related to
> your patch).
> > > > > >
> > > > > > > Add a property in the device-tree to notify MMC core about this
> > > > > > > configuration.
> > > > > > >
> > > > > > > Fixes: 089bee8dd119 ("arm64: dts: allwinner: h6: Introduce
> > > > > > > Beelink
> GS1 board")
> > > > > > > Signed-off-by: Clément Péron
> > > > > > > ---
> > > > > > > arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts | 1 +
> > > > > > > 1 file changed, 1 insertion(+)
> > > > > > >
> > > > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-
> gs1.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > > > > > > index 049c21718846..3f20d2c9 100644
> > > > > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > > > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > > > > > > @@ -145,6 +145,7 @@ {
> > > > > > > vqmmc-supply = <_bldo2>;
> > > > > > > non-removable;
> > > > > > > cap-mmc-hw-reset;
> > > > > > > + mmc-hs200-1_8v;
> > > > > > > bus-width = <8>;
> > > > > > > status = "okay";
> > > > > > > };
> > > > > >
> > > > > > I'm not really sure what you're trying to fix here, but as far as
> > > > > > MMC
> > > > > > goes, eMMC's can support io voltage of 3.3, 1.8 and 1.2V. Modes up
> until
> > > > > > HS DDR (50MHz in DDR) will use an IO voltage of 3.3V, higher speed
> modes
> > > > > > (HS200 and HS400) supporting 1.8V and 1.2V.
> > > > >
> > > > > Some users report that the eMMC is not working properly on their
> > > > > Beelink GS1 boards.
> > > > >
> > > > > > The mmc-hs200-1_8v property states that the MMC controller supports
> the
> > > > > > HS200 mode at 1.8V. Now, I can only assume that since BLDO2 is set
> up at
> > > > > > 1.8V then otherwise, the MMC core will rightfully decide to use the
> > > > > > highest supported mode. In this case, since the driver sets it, it
> would
> > > > > > be HS-DDR at 3.3V, which won't work with that fixed regulator.
> > > > > >
> > > > > > I can only assume that enabling HS200 at 1.8V only fixes the issue
> you
> > > > > > have because otherwise it would use HS-DDR at 3.3V, ie not actually
> > > > > > fixing the issue but sweeping it under the rug.
> > > > > >
> > > > > > Trying to add mmc-ddr-1_8v would be a good idea
> > > > >
> > > > > Thanks for the explanation, this is indeed the correct one.
> > > > > So It looks like the SDIO controller has an issue on some boards when
> > > > > using HS-DDR mode.
> > > > >
> > > > > Is this patch acceptable with the proper commit log?
> > > >
> > > > If HS-DDR works, yes, but I assume it doesn't?
> > >
> > > After discussing with Jernej about this issue, I understood that:
> > > - Automatic delay calibration is not implemented
> > > - We also miss some handling of DDR related bits in control reg
Re: [PATCH] arm64: dts: allwinner: pinetab: Drop unnecessary address/size-cells information
On Sun, Oct 11, 2020 at 11:15:14PM +0200, Clément Péron wrote: > make dtbs_check warm about unknown address/size-cells property in the > pinetab device-tree. > > This is because these information are not necessary. > > Drop them. > > Signed-off-by: Clément Péron Queued as a fix for 5.10 Thanks! Maxime signature.asc Description: PGP signature
Re: [PATCH] ARM: dts: sun8i: h2+: Enable optional SPI flash on Orange Pi Zero board
On Thu, Oct 08, 2020 at 07:40:44PM +0200, Michal Suchánek wrote:
> On Thu, Oct 08, 2020 at 07:14:54PM +0200, Maxime Ripard wrote:
> > On Thu, Oct 08, 2020 at 06:02:19PM +0200, Michal Suchánek wrote:
> > > On Thu, Oct 08, 2020 at 05:13:15PM +0200, Maxime Ripard wrote:
> > > > Hi,
> > > >
> > > > On Tue, Sep 29, 2020 at 10:30:25AM +0200, Michal Suchanek wrote:
> > > > > The flash is present on all new boards and users went out of their way
> > > > > to add it on the old ones.
> > > > >
> > > > > Enabling it makes a more reasonable default.
> > > > >
> > > > > Signed-off-by: Michal Suchanek
> > > > > ---
> > > > > arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts | 4 ++--
> > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> > > > > b/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> > > > > index f19ed981da9d..061d295bbba7 100644
> > > > > --- a/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> > > > > +++ b/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> > > > > @@ -163,8 +163,8 @@ {
> > > > > };
> > > > >
> > > > > {
> > > > > - /* Disable SPI NOR by default: it optional on Orange Pi Zero
> > > > > boards */
> > > > > - status = "disabled";
> > > > > + /* Enable optional SPI NOR by default */
> > > > > + status = "okay";
> > > > >
> > > > > flash@0 {
> > > > > #address-cells = <1>;
> > > >
> > > > Unfortunately, it's optional, so there's really no reason to enable it
> > > > all the time. If it's troublesome to users, then the distros or vendors
> > > > should make the changes necessary to the hardware, bootloader or their
> > > > documentation to make it easier for those users.
> > >
> > > I don't understand the reasoning. Why must it be disabled when optional?
> >
> > Think about it the other way around. If we enable everything that is
> > optional, we're going to have a multitude of conflicts everywhere, and
> > without a clear decision as to who is "best" and thus how we should
> > resolve it.
> Conflicts with what?
>
> The SPI0 bus is routed the the flash memory pads. Either there is the
> flash mounted or there are free pads. Nothing else on the board uses
> these pins. You could possily solder something else there but that's
> definitely not part of the board.
> >
> > On a separate platform, recently I've been using a VGA bridge for the
> > RaspberryPi that takes the UART pins as well. It's definitely optional,
> > should I enable it by default? At the same time, enabling by default the
> > UART is just as arbitrary and will result in people using the VGA bridge
> > to complain about their regression (rightfully so).
>
> That's completely different situation. That bridge is probably not even
> part of the board.
>
> >
> > So, really, if it's optional, it means that it not always there. If it's
> > not always there, it's meant to be supported by an overlay.
> >
> > > By the same reasoning there is no reason to disable it all the time.
> >
> > I'm not sure I follow you here. The least common denominator is that
> > it's not there, so it's not enabled.
>
> You have two options - have a flash mounted or not. You ask why enable
> flash when it is not always present. By the same logic I can ask why
> disable it when it is not always absent. Enabling is the more useful
> option because it degrades gracefully in the case it is not present. It
> does not work the other way around.
>
> >
> > > Also the boards that do not have the flsh are either broken or
> > > obsolete.
> >
> > Making general statements without arguments doesn't really make it true
> > though. Plenty of boards to have flash and are neither broken nor
> > obsolete.
>
> Cannot parse this.
"Plenty of boards do not have flash and are neither broken nor obsolete"
> >
> > > So most of the time enabling the flash chip is the right thing.
> > >
> > > Or do we need two DTBs like sun8i-h2-plus-orangepi-zero.dts and
> > > sun8i-h2-plus-orangepi-zero-no-spi-nor.dts
> >
> > No, you need sun8i-h2-plus-orangepi-zero plus an overlay for the
>
Re: [PATCH 2/2] dt-bindings: arm: sunxi: Fix Orange Pi Zero bindings
On Thu, Oct 08, 2020 at 08:40:06PM +0200, Michal Suchanek wrote: > There are two models of Orange Pi zero which are confusingly marketed > under the same name. Old model comes without a flash memory and current > model does have a flash memory. Add bindings for each model. > > Signed-off-by: Michal Suchanek Unfortunately, changing a compatible or a DT filename is not an option Maxime signature.asc Description: PGP signature
Re: [PATCH v7 01/14] ASoC: sun4i-i2s: Change set_chan_cfg() params
On Sun, Oct 11, 2020 at 10:22:11PM +0200, Clément Péron wrote: > As slots and slot_width can be set manually using set_tdm(). > These values are then kept in sun4i_i2s struct. > So we need to check if these values are set or not. > > This is not done actually and will trigger a bug. > For example, if we set to the simple soundcard in the device-tree > dai-tdm-slot-width = <32> and then start a stream using S16_LE, > currently we would calculate BCLK for 32-bit slots, but program > lrck_period for 16-bit slots, making the sample rate double what we > expected. > > To fix this, we need to check if these values are set or not but as > this logic is already done by the caller. Avoid duplicating this > logic and just pass the required values as params to set_chan_cfg(). > > Suggested-by: Samuel Holland > Signed-off-by: Clément Péron We still have an ongoing discussion on this one in the v6 Maxime signature.asc Description: PGP signature
Re: [PATCH v6 02/14] ASoC: sun4i-i2s: Change set_chan_cfg() params
On Tue, Oct 06, 2020 at 12:03:14AM -0500, Samuel Holland wrote:
> On 10/5/20 7:13 AM, Maxime Ripard wrote:
> > On Sat, Oct 03, 2020 at 04:19:38PM +0200, Clément Péron wrote:
> >> As slots and slot_width can be set manually using set_tdm().
> >> These values are then kept in sun4i_i2s struct.
> >> So we need to check if these values are setted or not
> >> in the struct.
> >>
> >> Avoid to check for this logic in set_chan_cfg(). This will
> >> duplicate the same check instead pass the required values
> >> as params to set_chan_cfg().
> >>
> >> This will also avoid a bug when we will enable 20/24bits support,
> >> i2s->slot_width is not actually used in the lrck_period computation.
> >>
> >> Suggested-by: Samuel Holland
> >> Signed-off-by: Clément Péron
> >> ---
> >> sound/soc/sunxi/sun4i-i2s.c | 36 ++--
> >> 1 file changed, 14 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> >> index c5ccd423e6d3..1f577dbc20a6 100644
> >> --- a/sound/soc/sunxi/sun4i-i2s.c
> >> +++ b/sound/soc/sunxi/sun4i-i2s.c
> >> @@ -177,8 +177,9 @@ struct sun4i_i2s_quirks {
> >>unsigned long (*get_bclk_parent_rate)(const struct sun4i_i2s *);
> >>s8 (*get_sr)(const struct sun4i_i2s *, int);
> >>s8 (*get_wss)(const struct sun4i_i2s *, int);
> >> - int (*set_chan_cfg)(const struct sun4i_i2s *,
> >> - const struct snd_pcm_hw_params *);
> >> + int (*set_chan_cfg)(const struct sun4i_i2s *i2s,
> >> + unsigned int channels, unsigned int slots,
> >> + unsigned int slot_width);
> >>int (*set_fmt)(const struct sun4i_i2s *, unsigned int);
> >> };
> >>
> >> @@ -414,10 +415,9 @@ static s8 sun8i_i2s_get_sr_wss(const struct sun4i_i2s
> >> *i2s, int width)
> >> }
> >>
> >> static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> >> -const struct snd_pcm_hw_params *params)
> >> +unsigned int channels, unsigned int slots,
> >> +unsigned int slot_width)
> >> {
> >> - unsigned int channels = params_channels(params);
> >> -
> >>/* Map the channels for playback and capture */
> >>regmap_write(i2s->regmap, SUN4I_I2S_TX_CHAN_MAP_REG, 0x76543210);
> >>regmap_write(i2s->regmap, SUN4I_I2S_RX_CHAN_MAP_REG, 0x3210);
> >> @@ -434,15 +434,11 @@ static int sun4i_i2s_set_chan_cfg(const struct
> >> sun4i_i2s *i2s,
> >> }
> >>
> >> static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> >> -const struct snd_pcm_hw_params *params)
> >> +unsigned int channels, unsigned int slots,
> >> +unsigned int slot_width)
> >> {
> >> - unsigned int channels = params_channels(params);
> >> - unsigned int slots = channels;
> >>unsigned int lrck_period;
> >>
> >> - if (i2s->slots)
> >> - slots = i2s->slots;
> >> -
> >>/* Map the channels for playback and capture */
> >>regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_MAP_REG, 0x76543210);
> >>regmap_write(i2s->regmap, SUN8I_I2S_RX_CHAN_MAP_REG, 0x76543210);
> >> @@ -467,11 +463,11 @@ static int sun8i_i2s_set_chan_cfg(const struct
> >> sun4i_i2s *i2s,
> >>case SND_SOC_DAIFMT_DSP_B:
> >>case SND_SOC_DAIFMT_LEFT_J:
> >>case SND_SOC_DAIFMT_RIGHT_J:
> >> - lrck_period = params_physical_width(params) * slots;
> >> + lrck_period = slot_width * slots;
> >>break;
> >>
> >>case SND_SOC_DAIFMT_I2S:
> >> - lrck_period = params_physical_width(params);
> >> + lrck_period = slot_width;
> >>break;
> >>
> >>default:
> >> @@ -490,15 +486,11 @@ static int sun8i_i2s_set_chan_cfg(const struct
> >> sun4i_i2s *i2s,
> >> }
> >>
> >> static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> >> -const struct snd_pcm_hw_params *params)
> >> +unsigned int channels, unsigned int slots,
> >> +
Re: [PATCH v6 02/14] ASoC: sun4i-i2s: Change set_chan_cfg() params
Hi,
On Mon, Oct 05, 2020 at 03:23:12PM +0200, Clément Péron wrote:
> On Mon, 5 Oct 2020 at 14:13, Maxime Ripard wrote:
> >
> > On Sat, Oct 03, 2020 at 04:19:38PM +0200, Clément Péron wrote:
> > > As slots and slot_width can be set manually using set_tdm().
> > > These values are then kept in sun4i_i2s struct.
> > > So we need to check if these values are setted or not
> > > in the struct.
> > >
> > > Avoid to check for this logic in set_chan_cfg(). This will
> > > duplicate the same check instead pass the required values
> > > as params to set_chan_cfg().
> > >
> > > This will also avoid a bug when we will enable 20/24bits support,
> > > i2s->slot_width is not actually used in the lrck_period computation.
> > >
> > > Suggested-by: Samuel Holland
> > > Signed-off-by: Clément Péron
> > > ---
> > > sound/soc/sunxi/sun4i-i2s.c | 36 ++--
> > > 1 file changed, 14 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > > index c5ccd423e6d3..1f577dbc20a6 100644
> > > --- a/sound/soc/sunxi/sun4i-i2s.c
> > > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > > @@ -177,8 +177,9 @@ struct sun4i_i2s_quirks {
> > > unsigned long (*get_bclk_parent_rate)(const struct sun4i_i2s *);
> > > s8 (*get_sr)(const struct sun4i_i2s *, int);
> > > s8 (*get_wss)(const struct sun4i_i2s *, int);
> > > - int (*set_chan_cfg)(const struct sun4i_i2s *,
> > > - const struct snd_pcm_hw_params *);
> > > + int (*set_chan_cfg)(const struct sun4i_i2s *i2s,
> > > + unsigned int channels, unsigned int slots,
> > > + unsigned int slot_width);
> > > int (*set_fmt)(const struct sun4i_i2s *, unsigned int);
> > > };
> > >
> > > @@ -414,10 +415,9 @@ static s8 sun8i_i2s_get_sr_wss(const struct
> > > sun4i_i2s *i2s, int width)
> > > }
> > >
> > > static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> > > - const struct snd_pcm_hw_params *params)
> > > + unsigned int channels, unsigned int slots,
> > > + unsigned int slot_width)
> > > {
> > > - unsigned int channels = params_channels(params);
> > > -
> > > /* Map the channels for playback and capture */
> > > regmap_write(i2s->regmap, SUN4I_I2S_TX_CHAN_MAP_REG, 0x76543210);
> > > regmap_write(i2s->regmap, SUN4I_I2S_RX_CHAN_MAP_REG, 0x3210);
> > > @@ -434,15 +434,11 @@ static int sun4i_i2s_set_chan_cfg(const struct
> > > sun4i_i2s *i2s,
> > > }
> > >
> > > static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> > > - const struct snd_pcm_hw_params *params)
> > > + unsigned int channels, unsigned int slots,
> > > + unsigned int slot_width)
> > > {
> > > - unsigned int channels = params_channels(params);
> > > - unsigned int slots = channels;
> > > unsigned int lrck_period;
> > >
> > > - if (i2s->slots)
> > > - slots = i2s->slots;
> > > -
> > > /* Map the channels for playback and capture */
> > > regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_MAP_REG, 0x76543210);
> > > regmap_write(i2s->regmap, SUN8I_I2S_RX_CHAN_MAP_REG, 0x76543210);
> > > @@ -467,11 +463,11 @@ static int sun8i_i2s_set_chan_cfg(const struct
> > > sun4i_i2s *i2s,
> > > case SND_SOC_DAIFMT_DSP_B:
> > > case SND_SOC_DAIFMT_LEFT_J:
> > > case SND_SOC_DAIFMT_RIGHT_J:
> > > - lrck_period = params_physical_width(params) * slots;
> > > + lrck_period = slot_width * slots;
> > > break;
> > >
> > > case SND_SOC_DAIFMT_I2S:
> > > - lrck_period = params_physical_width(params);
> > > + lrck_period = slot_width;
> > > break;
> > >
> > > default:
> > > @@ -490,15 +486,11 @@ static int sun8i_i2s_set_chan_cfg(const struct
> > > sun4i_i2s *i2s,
> > > }
> > >
> > > static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> > > -
Re: [PATCH v2] arm64: dts: allwinner: beelink-gs1: Update LED power node
Hi!
On Sun, Oct 11, 2020 at 11:22:37PM +0200, Clément Péron wrote:
> Beelink GS1 LED trigger a warning when running dtbs_check.
>
> Update the node with a valid pattern property.
>
> Also add the function and the color of the LED and drop the
> label which is deprecated.
>
> Signed-off-by: Clément Péron
> ---
> arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> index 3f7ceeb1a767..a364cb4e5b3f 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> @@ -7,6 +7,7 @@
> #include "sun50i-h6-cpu-opp.dtsi"
>
> #include
> +#include
>
> / {
> model = "Beelink GS1";
> @@ -43,8 +44,9 @@ ext_osc32k: ext_osc32k_clk {
> leds {
> compatible = "gpio-leds";
>
> - power {
> - label = "beelink:white:power";
> + led-0 {
> + function = LED_FUNCTION_POWER;
> + color = ;
> gpios = <_pio 0 4 GPIO_ACTIVE_HIGH>; /* PL4 */
> default-state = "on";
> };
Doesn't that also change the sysfs file that LED is exposed to the userspace
with?
Maxime
signature.asc
Description: PGP signature
Re: [PATCH 23/25] ASoC: sun8i-codec: Generalize AIF clock control
On Mon, Oct 05, 2020 at 11:51:08PM -0500, Samuel Holland wrote: > On 10/5/20 7:04 AM, Maxime Ripard wrote: > > Hi, > > > > On Wed, Sep 30, 2020 at 09:11:46PM -0500, Samuel Holland wrote: > >> The AIF clock control register has the same layout for all three AIFs. > >> The only difference between them is that AIF3 is missing some fields. We > >> can reuse the same register field definitions for all three registers, > >> and use the DAI ID to select the correct register address. > >> > >> Signed-off-by: Samuel Holland > >> --- > >> sound/soc/sunxi/sun8i-codec.c | 64 +++ > >> 1 file changed, 34 insertions(+), 30 deletions(-) > >> > >> diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c > >> index 032a3f714dbb..1c34502ac47a 100644 > >> --- a/sound/soc/sunxi/sun8i-codec.c > >> +++ b/sound/soc/sunxi/sun8i-codec.c > >> @@ -37,23 +37,23 @@ > >> #define SUN8I_MOD_CLK_ENA_DAC 2 > >> #define SUN8I_MOD_RST_CTL 0x014 > >> #define SUN8I_MOD_RST_CTL_AIF115 > >> #define SUN8I_MOD_RST_CTL_ADC 3 > >> #define SUN8I_MOD_RST_CTL_DAC 2 > >> #define SUN8I_SYS_SR_CTRL 0x018 > >> #define SUN8I_SYS_SR_CTRL_AIF1_FS 12 > >> #define SUN8I_SYS_SR_CTRL_AIF2_FS 8 > >> -#define SUN8I_AIF1CLK_CTRL0x040 > >> -#define SUN8I_AIF1CLK_CTRL_AIF1_MSTR_MOD 15 > >> -#define SUN8I_AIF1CLK_CTRL_AIF1_CLK_INV 13 > >> -#define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV 9 > >> -#define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV 6 > >> -#define SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ 4 > >> -#define SUN8I_AIF1CLK_CTRL_AIF1_DATA_FMT 2 > >> +#define SUN8I_AIF_CLK_CTRL(n) (0x040 * (1 + > >> (n))) > >> +#define SUN8I_AIF_CLK_CTRL_MSTR_MOD 15 > >> +#define SUN8I_AIF_CLK_CTRL_CLK_INV13 > >> +#define SUN8I_AIF_CLK_CTRL_BCLK_DIV 9 > >> +#define SUN8I_AIF_CLK_CTRL_LRCK_DIV 6 > >> +#define SUN8I_AIF_CLK_CTRL_WORD_SIZ 4 > >> +#define SUN8I_AIF_CLK_CTRL_DATA_FMT 2 > >> #define SUN8I_AIF1_ADCDAT_CTRL0x044 > >> #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0L_ENA 15 > >> #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0R_ENA 14 > >> #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0L_SRC 10 > >> #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0R_SRC 8 > >> #define SUN8I_AIF1_DACDAT_CTRL0x048 > >> #define SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0L_ENA 15 > >> #define SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0R_ENA 14 > >> @@ -83,21 +83,21 @@ > >> #define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF1DA1R 10 > >> #define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF2DACR 9 > >> #define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_ADCR 8 > >> > >> #define SUN8I_SYSCLK_CTL_AIF1CLK_SRC_MASK GENMASK(9, 8) > >> #define SUN8I_SYSCLK_CTL_AIF2CLK_SRC_MASK GENMASK(5, 4) > >> #define SUN8I_SYS_SR_CTRL_AIF1_FS_MASKGENMASK(15, 12) > >> #define SUN8I_SYS_SR_CTRL_AIF2_FS_MASKGENMASK(11, 8) > >> -#define SUN8I_AIF1CLK_CTRL_AIF1_CLK_INV_MASK GENMASK(14, 13) > >> -#define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV_MASK GENMASK(12, 9) > >> -#define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV_MASK GENMASK(8, 6) > >> -#define SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ_MASK GENMASK(5, 4) > >> -#define SUN8I_AIF1CLK_CTRL_AIF1_DATA_FMT_MASK GENMASK(3, 2) > >> +#define SUN8I_AIF_CLK_CTRL_CLK_INV_MASK GENMASK(14, 13) > >> +#define SUN8I_AIF_CLK_CTRL_BCLK_DIV_MASK GENMASK(12, 9) > >> +#define SUN8I_AIF_CLK_CTRL_LRCK_DIV_MASK GENMASK(8, 6) > >> +#define SUN8I_AIF_CLK_CTRL_WORD_SIZ_MASK GENMASK(5, 4) > >> +#define SUN8I_AIF_CLK_CTRL_DATA_FMT_MASK GENMASK(3, 2) > >> > >> #define SUN8I_CODEC_PASSTHROUGH_SAMPLE_RATE 48000 > >> > >> #define SUN8I_CODEC_PCM_FORMATS (SNDRV_PCM_FMTBIT_S8 |\ > >> SNDRV_PCM_FMTBIT_S16_LE |\ > >> SNDRV_PCM_FMTBIT_S20_LE |\ > >>
Re: [PATCH] dt-bindings: sound: sun8i-a33-codec: Add Allwinner A64 codec compatible fallback
Hi! On Sun, Oct 11, 2020 at 11:15:42PM +0200, Clément Péron wrote: > make dtbs_check report a warning because the documentation > for the A64 codec compatible is missing. > > The A64 codec compatible is actually a simple fallback to the A33. > > Reflect this in the dt-bindings Documentation. > > Signed-off-by: Clément Péron This patch is already in the ASoC tree and linux-next: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/Documentation/devicetree/bindings/sound/allwinner,sun8i-a33-codec.yaml?id=cef305d4eb0733f25215793ed30b056a7db9bb62 Maxime signature.asc Description: PGP signature
Re: [PATCH v2] arm64: dts: allwinner: h6: add eMMC voltage property for Beelink GS1
On Thu, Oct 08, 2020 at 10:00:06PM +0200, Clément Péron wrote:
> Hi Maxime,
>
> Adding linux-sunxi and Jernej Skrabec to this discussion.
>
> On Thu, 8 Oct 2020 at 17:10, Maxime Ripard wrote:
> >
> > Hi Clément,
> >
> > On Mon, Oct 05, 2020 at 08:47:19PM +0200, Clément Péron wrote:
> > > On Mon, 5 Oct 2020 at 11:21, Maxime Ripard wrote:
> > > >
> > > > Hi Clément,
> > > >
> > > > On Sat, Oct 03, 2020 at 11:20:01AM +0200, Clément Péron wrote:
> > > > > Sunxi MMC driver can't distinguish at runtime what's the I/O voltage
> > > > > for HS200 mode.
> > > >
> > > > Unfortunately, that's not true (or at least, that's not related to your
> > > > patch).
> > > >
> > > > > Add a property in the device-tree to notify MMC core about this
> > > > > configuration.
> > > > >
> > > > > Fixes: 089bee8dd119 ("arm64: dts: allwinner: h6: Introduce Beelink
> > > > > GS1 board")
> > > > > Signed-off-by: Clément Péron
> > > > > ---
> > > > > arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts | 1 +
> > > > > 1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > > > > b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > > > > index 049c21718846..3f20d2c9 100644
> > > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > > > > @@ -145,6 +145,7 @@ {
> > > > > vqmmc-supply = <_bldo2>;
> > > > > non-removable;
> > > > > cap-mmc-hw-reset;
> > > > > + mmc-hs200-1_8v;
> > > > > bus-width = <8>;
> > > > > status = "okay";
> > > > > };
> > > >
> > > > I'm not really sure what you're trying to fix here, but as far as MMC
> > > > goes, eMMC's can support io voltage of 3.3, 1.8 and 1.2V. Modes up until
> > > > HS DDR (50MHz in DDR) will use an IO voltage of 3.3V, higher speed modes
> > > > (HS200 and HS400) supporting 1.8V and 1.2V.
> > >
> > > Some users report that the eMMC is not working properly on their
> > > Beelink GS1 boards.
> > >
> > > > The mmc-hs200-1_8v property states that the MMC controller supports the
> > > > HS200 mode at 1.8V. Now, I can only assume that since BLDO2 is set up at
> > > > 1.8V then otherwise, the MMC core will rightfully decide to use the
> > > > highest supported mode. In this case, since the driver sets it, it would
> > > > be HS-DDR at 3.3V, which won't work with that fixed regulator.
> > > >
> > > > I can only assume that enabling HS200 at 1.8V only fixes the issue you
> > > > have because otherwise it would use HS-DDR at 3.3V, ie not actually
> > > > fixing the issue but sweeping it under the rug.
> > > >
> > > > Trying to add mmc-ddr-1_8v would be a good idea
> > >
> > > Thanks for the explanation, this is indeed the correct one.
> > > So It looks like the SDIO controller has an issue on some boards when
> > > using HS-DDR mode.
> > >
> > > Is this patch acceptable with the proper commit log?
> >
> > If HS-DDR works, yes, but I assume it doesn't?
>
> After discussing with Jernej about this issue, I understood that:
> - Automatic delay calibration is not implemented
> - We also miss some handling of DDR related bits in control register
>
> So none of H5/H6 boards should actually work.
> (Some 'lucky' boards seem to work enough to switch to HS200 mode...)
>
> To "fix" this the H5 disable the HS-DDR mode in sunxi mmc driver :
> https://github.com/torvalds/linux/blob/master/drivers/mmc/host/sunxi-mmc.c#L1409
I find it suspicious that some boards would have traces not good enough
for HS-DDR (50MHz in DDR) but would work fine in HS200 (200MHz in SDR).
If there's some mismatch on the traces, it will only be worse in HS200.
And for the delay calibration, iirc, that's only necessary for HS400
that we don't support?
> I'm not sure about A64 but it looks like the property "mmc-hs200-1_8v"
> for the PineBook shows the same issue.
>
> The proper way would of course be to implement the missing feature
> mentioned above.
> But this could take some time and as the eMMC driver is actually
> broken wouldn't it be better to disable the HS-DDR for H6 in the mmc
> driver like it's done for H5 ?
Have you tested with only the mmc-ddr-1_8v property?
Maxime
signature.asc
Description: PGP signature
Re: [PATCH] ARM: dts: sun8i: h2+: Enable optional SPI flash on Orange Pi Zero board
On Thu, Oct 08, 2020 at 06:02:19PM +0200, Michal Suchánek wrote:
> On Thu, Oct 08, 2020 at 05:13:15PM +0200, Maxime Ripard wrote:
> > Hi,
> >
> > On Tue, Sep 29, 2020 at 10:30:25AM +0200, Michal Suchanek wrote:
> > > The flash is present on all new boards and users went out of their way
> > > to add it on the old ones.
> > >
> > > Enabling it makes a more reasonable default.
> > >
> > > Signed-off-by: Michal Suchanek
> > > ---
> > > arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> > > b/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> > > index f19ed981da9d..061d295bbba7 100644
> > > --- a/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> > > +++ b/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> > > @@ -163,8 +163,8 @@ {
> > > };
> > >
> > > {
> > > - /* Disable SPI NOR by default: it optional on Orange Pi Zero boards */
> > > - status = "disabled";
> > > + /* Enable optional SPI NOR by default */
> > > + status = "okay";
> > >
> > > flash@0 {
> > > #address-cells = <1>;
> >
> > Unfortunately, it's optional, so there's really no reason to enable it
> > all the time. If it's troublesome to users, then the distros or vendors
> > should make the changes necessary to the hardware, bootloader or their
> > documentation to make it easier for those users.
>
> I don't understand the reasoning. Why must it be disabled when optional?
Think about it the other way around. If we enable everything that is
optional, we're going to have a multitude of conflicts everywhere, and
without a clear decision as to who is "best" and thus how we should
resolve it.
On a separate platform, recently I've been using a VGA bridge for the
RaspberryPi that takes the UART pins as well. It's definitely optional,
should I enable it by default? At the same time, enabling by default the
UART is just as arbitrary and will result in people using the VGA bridge
to complain about their regression (rightfully so).
So, really, if it's optional, it means that it not always there. If it's
not always there, it's meant to be supported by an overlay.
> By the same reasoning there is no reason to disable it all the time.
I'm not sure I follow you here. The least common denominator is that
it's not there, so it's not enabled.
> Also the boards that do not have the flsh are either broken or
> obsolete.
Making general statements without arguments doesn't really make it true
though. Plenty of boards to have flash and are neither broken nor
obsolete.
> So most of the time enabling the flash chip is the right thing.
>
> Or do we need two DTBs like sun8i-h2-plus-orangepi-zero.dts and
> sun8i-h2-plus-orangepi-zero-no-spi-nor.dts
No, you need sun8i-h2-plus-orangepi-zero plus an overlay for the
SPI-NOR.
> There is no way to change the setting on a runnig system, the pins are
> routed to the flash pads anyway so are not usable for anything else. The
> only thing that happens on boards that do not have the flash is kernel
> probing it and complaining that the ID 00 00 00 is not valid SPI NOR
> flash memory ID.
We have people reporting bugs about completely innocuous error messages
without any side effects already. An error about a missing or broken
storage device will surely raise some eyebrows.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH] ARM: dts: sun8i: h2+: Enable optional SPI flash on Orange Pi Zero board
Hi,
On Tue, Sep 29, 2020 at 10:30:25AM +0200, Michal Suchanek wrote:
> The flash is present on all new boards and users went out of their way
> to add it on the old ones.
>
> Enabling it makes a more reasonable default.
>
> Signed-off-by: Michal Suchanek
> ---
> arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> b/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> index f19ed981da9d..061d295bbba7 100644
> --- a/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> +++ b/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> @@ -163,8 +163,8 @@ {
> };
>
> {
> - /* Disable SPI NOR by default: it optional on Orange Pi Zero boards */
> - status = "disabled";
> + /* Enable optional SPI NOR by default */
> + status = "okay";
>
> flash@0 {
> #address-cells = <1>;
Unfortunately, it's optional, so there's really no reason to enable it
all the time. If it's troublesome to users, then the distros or vendors
should make the changes necessary to the hardware, bootloader or their
documentation to make it easier for those users.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v2] arm64: dts: allwinner: h6: add eMMC voltage property for Beelink GS1
Hi Clément,
On Mon, Oct 05, 2020 at 08:47:19PM +0200, Clément Péron wrote:
> On Mon, 5 Oct 2020 at 11:21, Maxime Ripard wrote:
> >
> > Hi Clément,
> >
> > On Sat, Oct 03, 2020 at 11:20:01AM +0200, Clément Péron wrote:
> > > Sunxi MMC driver can't distinguish at runtime what's the I/O voltage
> > > for HS200 mode.
> >
> > Unfortunately, that's not true (or at least, that's not related to your
> > patch).
> >
> > > Add a property in the device-tree to notify MMC core about this
> > > configuration.
> > >
> > > Fixes: 089bee8dd119 ("arm64: dts: allwinner: h6: Introduce Beelink GS1
> > > board")
> > > Signed-off-by: Clément Péron
> > > ---
> > > arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > > b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > > index 049c21718846..3f20d2c9 100644
> > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > > @@ -145,6 +145,7 @@ {
> > > vqmmc-supply = <_bldo2>;
> > > non-removable;
> > > cap-mmc-hw-reset;
> > > + mmc-hs200-1_8v;
> > > bus-width = <8>;
> > > status = "okay";
> > > };
> >
> > I'm not really sure what you're trying to fix here, but as far as MMC
> > goes, eMMC's can support io voltage of 3.3, 1.8 and 1.2V. Modes up until
> > HS DDR (50MHz in DDR) will use an IO voltage of 3.3V, higher speed modes
> > (HS200 and HS400) supporting 1.8V and 1.2V.
>
> Some users report that the eMMC is not working properly on their
> Beelink GS1 boards.
>
> > The mmc-hs200-1_8v property states that the MMC controller supports the
> > HS200 mode at 1.8V. Now, I can only assume that since BLDO2 is set up at
> > 1.8V then otherwise, the MMC core will rightfully decide to use the
> > highest supported mode. In this case, since the driver sets it, it would
> > be HS-DDR at 3.3V, which won't work with that fixed regulator.
> >
> > I can only assume that enabling HS200 at 1.8V only fixes the issue you
> > have because otherwise it would use HS-DDR at 3.3V, ie not actually
> > fixing the issue but sweeping it under the rug.
> >
> > Trying to add mmc-ddr-1_8v would be a good idea
>
> Thanks for the explanation, this is indeed the correct one.
> So It looks like the SDIO controller has an issue on some boards when
> using HS-DDR mode.
>
> Is this patch acceptable with the proper commit log?
If HS-DDR works, yes, but I assume it doesn't?
Maxime
signature.asc
Description: PGP signature
Re: linux-next: manual merge of the sunxi tree with the arm-soc tree
Hi Stephen,
On Tue, Oct 06, 2020 at 02:56:37PM +1100, Stephen Rothwell wrote:
> Hi all,
>
> Today's linux-next merge of the sunxi tree got a conflict in:
>
> arch/arm64/boot/dts/allwinner/sun50i-a100.dtsi
>
> between commit:
>
> 0dea1794f3b4 ("arm64: allwinner: A100: add the basical Allwinner A100 DTSI
> file")
>
> from the arm-soc tree and commit:
>
> 7e66a778cb8b ("arm64: allwinner: A100: add the basical Allwinner A100 DTSI
> file")
>
> from the sunxi tree.
>
> These are 2 versions of the same patch. For now I am just using the
> version in the arm-soc tree ... please sort this out.
>
> I fixed it up (see above) and can carry the fix as necessary. This
> is now fixed as far as linux-next is concerned, but any non trivial
> conflicts should be mentioned to your upstream maintainer when your tree
> is submitted for merging. You may also want to consider cooperating
> with the maintainer of the conflicting tree to minimise any particularly
> complex conflicts.
The branch in arm-soc has a build breakage (that doesn't happen in
linux-next since the clk tree has the commit to fix it) so I sent a new
PR
Once that PR is in arm-soc, I guess that merge issue will go away
maxime
signature.asc
Description: PGP signature
Re: [PATCH 20/25] ASoC: sun8i-codec: Protect the clock rate while streams are open
On Mon, Oct 05, 2020 at 11:43:51PM -0500, Samuel Holland wrote:
> On 10/5/20 7:01 AM, Maxime Ripard wrote:
> > On Wed, Sep 30, 2020 at 09:11:43PM -0500, Samuel Holland wrote:
> >> The codec's clock input is shared among all AIFs, and shared with other
> >> audio-related hardware in the SoC, including I2S and SPDIF controllers.
> >> To ensure sample rates selected by userspace or by codec2codec DAI links
> >> are maintained, the clock rate must be protected while it is in use.
> >>
> >> Signed-off-by: Samuel Holland
> >> ---
> >> sound/soc/sunxi/sun8i-codec.c | 25 ++---
> >> 1 file changed, 22 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
> >> index 501af64d43a0..86065bee7cd3 100644
> >> --- a/sound/soc/sunxi/sun8i-codec.c
> >> +++ b/sound/soc/sunxi/sun8i-codec.c
> >> @@ -416,27 +416,32 @@ static int sun8i_codec_get_lrck_div_order(unsigned
> >> int slots,
> >>unsigned int div = slots * slot_width;
> >>
> >>if (div < 16 || div > 256)
> >>return -EINVAL;
> >>
> >>return order_base_2(div);
> >> }
> >>
> >> +static unsigned int sun8i_codec_get_sysclk_rate(unsigned int sample_rate)
> >> +{
> >> + return sample_rate % 4000 ? 22579200 : 24576000;
> >> +}
> >> +
> >> static int sun8i_codec_hw_params(struct snd_pcm_substream *substream,
> >> struct snd_pcm_hw_params *params,
> >> struct snd_soc_dai *dai)
> >> {
> >>struct sun8i_codec *scodec = snd_soc_dai_get_drvdata(dai);
> >>struct sun8i_codec_aif *aif = >aifs[dai->id];
> >>unsigned int sample_rate = params_rate(params);
> >>unsigned int slots = aif->slots ?: params_channels(params);
> >>unsigned int slot_width = aif->slot_width ?: params_width(params);
> >> - unsigned int sysclk_rate = clk_get_rate(scodec->clk_module);
> >> - int lrck_div_order, word_size;
> >> + unsigned int sysclk_rate = sun8i_codec_get_sysclk_rate(sample_rate);
> >> + int lrck_div_order, ret, word_size;
> >>u8 bclk_div;
> >>
> >>/* word size */
> >>switch (params_width(params)) {
> >>case 8:
> >>word_size = 0x0;
> >>break;
> >>case 16:
> >> @@ -466,17 +471,30 @@ static int sun8i_codec_hw_params(struct
> >> snd_pcm_substream *substream,
> >> (lrck_div_order - 4) <<
> >> SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV);
> >>
> >>/* BCLK divider (SYSCLK/BCLK ratio) */
> >>bclk_div = sun8i_codec_get_bclk_div(sysclk_rate, lrck_div_order,
> >> sample_rate);
> >>regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
> >> SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV_MASK,
> >> bclk_div << SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV);
> >>
> >> - if (!aif->open_streams) {
> >> + /* SYSCLK rate */
> >> + if (aif->open_streams) {
> >> + ret = clk_set_rate(scodec->clk_module, sysclk_rate);
> >> + if (ret < 0)
> >> + return ret;
> >> + } else {
> >> + ret = clk_set_rate_exclusive(scodec->clk_module, sysclk_rate);
> >
> > It's not really clear to me why we wouldn't want to always protect the
> > clock rate here?
>
> From Documentation/sound/kernel-api/writing-an-alsa-driver.rst:
>
> hw_params callback
> ...
> Note that this and ``prepare`` callbacks may be called multiple
> times per initialization. For example, the OSS emulation may call
> these callbacks at each change via its ioctl.
>
> Clock rate protection is reference counted, so we must only take one
> reference (or at least a known number of references) per stream.
Ah, right.
Can you add a comment to make that more obvious?
> >> + if (ret == -EBUSY)
> >> + dev_err(dai->dev, "%s: clock is busy! Sample rate %u Hz
> >> "
> >> + "conflicts with other audio streams.\n",
> >
> > This string creates a checkpatch warning.
>
> I will put it on one line, though >100 columns is also a checkpatch warning.
Yeah, but in general having an error on a single line is more important.
That way you can then grep for that error message
Maxime
signature.asc
Description: PGP signature
Re: [PATCH 11/25] ASoC: sun8i-codec: Enable all supported clock inversions
On Mon, Oct 05, 2020 at 11:29:51PM -0500, Samuel Holland wrote:
>
> On 10/5/20 6:30 AM, Maxime Ripard wrote:
> > On Wed, Sep 30, 2020 at 09:11:34PM -0500, Samuel Holland wrote:
> > > When using the I2S, LEFT_J, or RIGHT_J format, the hardware supports
> > > independent BCLK and LRCK inversion control. When using DSP_A or DSP_B,
> > > LRCK inversion is not supported. The register bit is repurposed to
> > > select between DSP_A and DSP_B. Extend the driver to support this.
> > >
> > > Signed-off-by: Samuel Holland
> > > ---
> > > sound/soc/sunxi/sun8i-codec.c | 57 +++
> > > 1 file changed, 37 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
> > > index 0b713b2a2028..506420fb355c 100644
> > > --- a/sound/soc/sunxi/sun8i-codec.c
> > > +++ b/sound/soc/sunxi/sun8i-codec.c
> > > @@ -39,18 +39,17 @@
> > > #define SUN8I_MOD_RST_CTL_AIF1 15
> > > #define SUN8I_MOD_RST_CTL_ADC 3
> > > #define SUN8I_MOD_RST_CTL_DAC 2
> > > #define SUN8I_SYS_SR_CTRL 0x018
> > > #define SUN8I_SYS_SR_CTRL_AIF1_FS 12
> > > #define SUN8I_SYS_SR_CTRL_AIF2_FS 8
> > > #define SUN8I_AIF1CLK_CTRL 0x040
> > > #define SUN8I_AIF1CLK_CTRL_AIF1_MSTR_MOD15
> > > -#define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_INV 14
> > > -#define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV 13
> > > +#define SUN8I_AIF1CLK_CTRL_AIF1_CLK_INV 13
> > > #define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV9
> > > #define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV6
> > > #define SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ4
> > > #define SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ_16 (1 << 4)
> > > #define SUN8I_AIF1CLK_CTRL_AIF1_DATA_FMT2
> > > #define SUN8I_AIF1_ADCDAT_CTRL 0x044
> > > #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0L_ENA15
> > > #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0R_ENA14
> > > @@ -85,16 +84,17 @@
> > > #define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF1DA1R 10
> > > #define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF2DACR 9
> > > #define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_ADCR 8
> > > #define SUN8I_SYSCLK_CTL_AIF1CLK_SRC_MASK GENMASK(9, 8)
> > > #define SUN8I_SYSCLK_CTL_AIF2CLK_SRC_MASK GENMASK(5, 4)
> > > #define SUN8I_SYS_SR_CTRL_AIF1_FS_MASK GENMASK(15, 12)
> > > #define SUN8I_SYS_SR_CTRL_AIF2_FS_MASK GENMASK(11, 8)
> > > +#define SUN8I_AIF1CLK_CTRL_AIF1_CLK_INV_MASK GENMASK(14, 13)
> > > #define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV_MASK GENMASK(12, 9)
> > > #define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV_MASK GENMASK(8, 6)
> > > #define SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ_MASK GENMASK(5, 4)
> > > #define SUN8I_AIF1CLK_CTRL_AIF1_DATA_FMT_MASK GENMASK(3, 2)
> > > enum {
> > > AIF1,
> > > NAIFS
> > > @@ -168,17 +168,17 @@ static int sun8i_codec_get_hw_rate(struct
> > > snd_pcm_hw_params *params)
> > > default:
> > > return -EINVAL;
> > > }
> > > }
> > > static int sun8i_codec_set_fmt(struct snd_soc_dai *dai, unsigned int
> > > fmt)
> > > {
> > > struct sun8i_codec *scodec = snd_soc_dai_get_drvdata(dai);
> > > - u32 format, value;
> > > + u32 format, invert, value;
> > > /* clock masters */
> > > switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> > > case SND_SOC_DAIFMT_CBS_CFS: /* Codec slave, DAI master */
> > > value = 0x1;
> > > break;
> > > case SND_SOC_DAIFMT_CBM_CFM: /* Codec Master, DAI slave */
> > > value = 0x0;
> > > @@ -197,55 +197,72 @@ static int sun8i_codec_set_fmt(struct snd_soc_dai
> > > *dai, unsigned int fmt)
> > > break;
> > > case SND_SOC_DAIFMT_LEFT_J:
> > > format = 0x1;
> > > break;
> > > case SND_SOC_DAIFMT_RIGHT_J:
> > > format = 0x2;
> > > break;
> > >
Re: [PATCH v5 00/80] drm/vc4: Support BCM2711 Display Pipeline
On Wed, Sep 16, 2020 at 06:57:05PM +0200, Maxime Ripard wrote: > On Mon, Sep 14, 2020 at 07:14:11PM +0900, Hoegeun Kwon wrote: > > Hi Maxime, > > > > On 9/8/20 9:00 PM, Maxime Ripard wrote: > > > Hi Hoegeun, > > > > > > On Mon, Sep 07, 2020 at 08:49:12PM +0900, Hoegeun Kwon wrote: > > >> On 9/3/20 5:00 PM, Maxime Ripard wrote: > > >>> Hi everyone, > > >>> > > >>> Here's a (pretty long) series to introduce support in the VC4 DRM driver > > >>> for the display pipeline found in the BCM2711 (and thus the RaspberryPi > > >>> 4). > > >>> > > >>> The main differences are that there's two HDMI controllers and that > > >>> there's > > >>> more pixelvalve now. Those pixelvalve come with a mux in the HVS that > > >>> still > > >>> have only 3 FIFOs. Both of those differences are breaking a bunch of > > >>> expectations in the driver, so we first need a good bunch of cleanup and > > >>> reworks to introduce support for the new controllers. > > >>> > > >>> Similarly, the HDMI controller has all its registers shuffled and split > > >>> in > > >>> multiple controllers now, so we need a bunch of changes to support this > > >>> as > > >>> well. > > >>> > > >>> Only the HDMI support is enabled for now (even though the DPI and DSI > > >>> outputs have been tested too). > > >>> > > >>> Let me know if you have any comments > > >>> Maxime > > >>> > > >>> Cc: bcm-kernel-feedback-l...@broadcom.com > > >>> Cc: devicet...@vger.kernel.org > > >>> Cc: Kamal Dasu > > >>> Cc: Philipp Zabel > > >>> Cc: Rob Herring > > >>> Cc: Stephen Boyd > > >>> > > >>> Changes from v4: > > >>> - Rebased on top of next-20200828 > > >>> - Collected the various tags > > >>> - Fixed some issues with 4k support and dual output (thanks > > >>> Hoegeun!) > > >> Thanks for your v5 patchset. > > >> > > >> I tested all patches based on the next-20200812. > > > Thanks again for testing all the patches > > > > > >> Everything else is fine, but the dual hdmi modetest doesn't work well in > > >> my > > >> environment... > > >> > > >> In my environment, dsi is not connected, I have seen your answer[1]. > > > Can you share a bit more your setup? What monitors are being connected > > > to each HDMI port? Do you hotplug any? > > Yes, Monitors are being connected to each HDMI ports. (did not use hotplug) > > > > When booting, both HDMI-0 and 1 are recognized and the kernel log is output. > > But after run modetest on HDMI-0(works) and modetest on HDMI-1(works), > > crtc timed out occurs on HDMI-0 and does not work. > > > > When HDMI-0 is not working we do a modetest on HDMI-0, it will work agin > > after about 40 sec. > > > > Below is the log for modetest. > > > > > > root:~> modetest -Mvc4 -s 32:1280x720 - HDMI-0 works > > setting mode 1280x720-60Hz@XR24 on connectors 32, crtc 64 > > failed to set gamma: Invalid argument > > > > root:~> modetest -Mvc4 -s 32:1280x720 - HDMI-0 works > > setting mode 1280x720-60Hz@XR24 on connectors 32, crtc 64 > > failed to set gamma: Invalid argument > > > > root:~> modetest -Mvc4 -s 38:1280x720 - HDMI-1 works > > setting mode 1280x720-60Hz@XR24 on connectors 38, crtc 69 > > failed to set gamma: Invalid argument > > > > - Crtc timed out occurs on HDMI-0 and > > does not work. > > > > [ 71.134283] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* > > [CRTC:64:crtc-3] flip_done timed out > > [ 81.374296] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* > > [CRTC:64:crtc-3] flip_done timed out > > [ 91.618380] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* > > [CONNECTOR:32:HDMI-A-1] flip_done timed out > > [ 101.854274] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* > > [PLANE:60:plane-3] flip_done timed out > > > > [ 112.094271] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* > > [CRTC:64:crtc-3] flip_done timed out > > [ 122.590311] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* > > [CRTC:64:crtc-3] flip
Re: [PATCH v5 80/80] ARM: dts: bcm2711: Enable the display pipeline
Hi Dave, On Fri, Oct 02, 2020 at 04:57:05PM +0100, Dave Stevenson wrote: > Hi Maxime > > On Fri, 2 Oct 2020 at 16:19, Maxime Ripard wrote: > > > > Hi Tim, > > > > On Thu, Oct 01, 2020 at 11:15:46AM +0100, Tim Gover wrote: > > > hdmi_enable_4k60=1 causes the firmware to select 3.3 GHz for the PLLC > > > VCO to support a core-frequency of 550 MHz which is the minimum > > > frequency required by the HVS at 4Kp60. The side effect is that if the > > > display clock requirements are lower than 4Kp60 then you will see > > > different core frequencies selected by DVFS. > > > > > > If enable_uart=1 and the mini-uart is selected (default unless > > > bluetooth is disabled) then the firmware will pin the core-frequency > > > to either core_freq max (500 or 550). Although, I think there is a way > > > of pinning it to a lower fixed frequency. > > > > > > The table in overclocking.md defines options for setting the maximum > > > core frequency but unless core_freq_min is specified DVFS will > > > automatically pick the lowest idle frequency required by the display > > > resolution. > > > > I'm wondering if there's some way to detect this from Linux? I guess it > > would be nice to be able to at least detect a broken config to warn / > > prevent an user that their situation is not going to be reliable / work > > really well (like if they have a 4k display without hdmi_enable_4kp60 > > set, or the issue we're discussing here) > > The main filter in the firmware is the parameter > hdmi_pixel_freq_limit. That can either be set manually from > config.txt, or defaults appropriately based on hdmi_enable_4kp60. > Under firmware_kms [1] I read back those values to use as a filter > within crtc_mode_valid[2]. > I can't think of a nice way of exposing that without the vc4 driver > gaining a DT link to the firmware, and that starts to get ugly. I had in mind something like if the clock driver can infer that somehow through some the boundaries reported by the firmware maybe? IIRC, hdmi_enable_4kp60 will already change the max frequency reported to 550MHz instead of 500MHz Maxime signature.asc Description: PGP signature
Re: [PATCH v6 08/14] ASoC: sun4i-i2s: fix coding-style for callback definition
On Sat, Oct 03, 2020 at 04:19:44PM +0200, Clément Péron wrote: > Checkpatch script produces warning: > WARNING: function definition argument 'const struct sun4i_i2s *' > should also have an identifier name. > > Let's fix this by adding identifier name to get_bclk_parent_rate() > and set_fmt() callback definition. > > Signed-off-by: Clément Péron Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH v6 03/14] ASoC: sun4i-i2s: Change get_sr() and get_wss() to be more explicit
On Sat, Oct 03, 2020 at 04:19:39PM +0200, Clément Péron wrote:
> We are actually using a complex formula to just return a bunch of
> simple values. Also this formula is wrong for sun4i when calling
> get_wss() the function return 4 instead of 3.
>
> Replace this with a simpler switch case.
>
> Also drop the i2s params which is unused and return a simple int as
> returning an error code could be out of range for an s8 and there is
> no optim to return a s8 here.
>
> Fixes: 619c15f7fac9 ("ASoC: sun4i-i2s: Change SR and WSS computation")
> Reviewed-by: Chen-Yu Tsai
> Signed-off-by: Clément Péron
> ---
> sound/soc/sunxi/sun4i-i2s.c | 69 +++--
> 1 file changed, 44 insertions(+), 25 deletions(-)
>
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index 1f577dbc20a6..8e497fb3de09 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -175,8 +175,8 @@ struct sun4i_i2s_quirks {
> unsigned intnum_mclk_dividers;
>
> unsigned long (*get_bclk_parent_rate)(const struct sun4i_i2s *);
> - s8 (*get_sr)(const struct sun4i_i2s *, int);
> - s8 (*get_wss)(const struct sun4i_i2s *, int);
> + int (*get_sr)(unsigned int width);
> + int (*get_wss)(unsigned int width);
> int (*set_chan_cfg)(const struct sun4i_i2s *i2s,
> unsigned int channels, unsigned int slots,
> unsigned int slot_width);
> @@ -381,37 +381,56 @@ static int sun4i_i2s_set_clk_rate(struct snd_soc_dai
> *dai,
> return 0;
> }
>
> -static s8 sun4i_i2s_get_sr(const struct sun4i_i2s *i2s, int width)
> +static int sun4i_i2s_get_sr(unsigned int width)
> {
> - if (width < 16 || width > 24)
> - return -EINVAL;
> -
> - if (width % 4)
> - return -EINVAL;
> + switch (width) {
> + case 16:
> + return 0x0;
> + case 20:
> + return 0x1;
> + case 24:
> + return 0x2;
> + }
>
> - return (width - 16) / 4;
> + return -EINVAL;
> }
>
> -static s8 sun4i_i2s_get_wss(const struct sun4i_i2s *i2s, int width)
> +static int sun4i_i2s_get_wss(unsigned int width)
> {
> - if (width < 16 || width > 32)
> - return -EINVAL;
> -
> - if (width % 4)
> - return -EINVAL;
> + switch (width) {
> + case 16:
> + return 0x0;
> + case 20:
> + return 0x1;
> + case 24:
> + return 0x2;
> + case 32:
> + return 0x3;
> + }
Like I said in the previous version, I'm not really sure why we need to
use the hexadecimal representation here?
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v6 02/14] ASoC: sun4i-i2s: Change set_chan_cfg() params
On Sat, Oct 03, 2020 at 04:19:38PM +0200, Clément Péron wrote:
> As slots and slot_width can be set manually using set_tdm().
> These values are then kept in sun4i_i2s struct.
> So we need to check if these values are setted or not
> in the struct.
>
> Avoid to check for this logic in set_chan_cfg(). This will
> duplicate the same check instead pass the required values
> as params to set_chan_cfg().
>
> This will also avoid a bug when we will enable 20/24bits support,
> i2s->slot_width is not actually used in the lrck_period computation.
>
> Suggested-by: Samuel Holland
> Signed-off-by: Clément Péron
> ---
> sound/soc/sunxi/sun4i-i2s.c | 36 ++--
> 1 file changed, 14 insertions(+), 22 deletions(-)
>
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index c5ccd423e6d3..1f577dbc20a6 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -177,8 +177,9 @@ struct sun4i_i2s_quirks {
> unsigned long (*get_bclk_parent_rate)(const struct sun4i_i2s *);
> s8 (*get_sr)(const struct sun4i_i2s *, int);
> s8 (*get_wss)(const struct sun4i_i2s *, int);
> - int (*set_chan_cfg)(const struct sun4i_i2s *,
> - const struct snd_pcm_hw_params *);
> + int (*set_chan_cfg)(const struct sun4i_i2s *i2s,
> + unsigned int channels, unsigned int slots,
> + unsigned int slot_width);
> int (*set_fmt)(const struct sun4i_i2s *, unsigned int);
> };
>
> @@ -414,10 +415,9 @@ static s8 sun8i_i2s_get_sr_wss(const struct sun4i_i2s
> *i2s, int width)
> }
>
> static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> - const struct snd_pcm_hw_params *params)
> + unsigned int channels, unsigned int slots,
> + unsigned int slot_width)
> {
> - unsigned int channels = params_channels(params);
> -
> /* Map the channels for playback and capture */
> regmap_write(i2s->regmap, SUN4I_I2S_TX_CHAN_MAP_REG, 0x76543210);
> regmap_write(i2s->regmap, SUN4I_I2S_RX_CHAN_MAP_REG, 0x3210);
> @@ -434,15 +434,11 @@ static int sun4i_i2s_set_chan_cfg(const struct
> sun4i_i2s *i2s,
> }
>
> static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> - const struct snd_pcm_hw_params *params)
> + unsigned int channels, unsigned int slots,
> + unsigned int slot_width)
> {
> - unsigned int channels = params_channels(params);
> - unsigned int slots = channels;
> unsigned int lrck_period;
>
> - if (i2s->slots)
> - slots = i2s->slots;
> -
> /* Map the channels for playback and capture */
> regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_MAP_REG, 0x76543210);
> regmap_write(i2s->regmap, SUN8I_I2S_RX_CHAN_MAP_REG, 0x76543210);
> @@ -467,11 +463,11 @@ static int sun8i_i2s_set_chan_cfg(const struct
> sun4i_i2s *i2s,
> case SND_SOC_DAIFMT_DSP_B:
> case SND_SOC_DAIFMT_LEFT_J:
> case SND_SOC_DAIFMT_RIGHT_J:
> - lrck_period = params_physical_width(params) * slots;
> + lrck_period = slot_width * slots;
> break;
>
> case SND_SOC_DAIFMT_I2S:
> - lrck_period = params_physical_width(params);
> + lrck_period = slot_width;
> break;
>
> default:
> @@ -490,15 +486,11 @@ static int sun8i_i2s_set_chan_cfg(const struct
> sun4i_i2s *i2s,
> }
>
> static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> - const struct snd_pcm_hw_params *params)
> + unsigned int channels, unsigned int slots,
> + unsigned int slot_width)
> {
> - unsigned int channels = params_channels(params);
> - unsigned int slots = channels;
> unsigned int lrck_period;
>
> - if (i2s->slots)
> - slots = i2s->slots;
> -
> /* Map the channels for playback and capture */
> regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP0_REG, 0xFEDCBA98);
> regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP1_REG, 0x76543210);
> @@ -525,11 +517,11 @@ static int sun50i_h6_i2s_set_chan_cfg(const struct
> sun4i_i2s *i2s,
> case SND_SOC_DAIFMT_DSP_B:
> case SND_SOC_DAIFMT_LEFT_J:
> case SND_SOC_DAIFMT_RIGHT_J:
> - lrck_period = params_physical_width(params) * slots;
> + lrck_period = slot_width * slots;
> break;
>
> case SND_SOC_DAIFMT_I2S:
> - lrck_period = params_physical_width(params);
> + lrck_period = slot_width;
> break;
>
> default:
> @@ -565,7 +557,7 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream
> *substream,
> if (i2s->slot_width)
>
Re: [PATCH 24/25] ASoC: sun8i-codec: Add a DAI, widgets, and routes for AIF2
On Wed, Sep 30, 2020 at 09:11:47PM -0500, Samuel Holland wrote: > This adds support for AIF2, which is stereo and has full clocking > capability, making it very similar to AIF1. > > Signed-off-by: Samuel Holland Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH 23/25] ASoC: sun8i-codec: Generalize AIF clock control
value = 0x1; > break; > case SND_SOC_DAIFMT_CBM_CFM: /* Codec Master, DAI slave */ > value = 0x0; > break; > default: > return -EINVAL; > } > - regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL, > -BIT(SUN8I_AIF1CLK_CTRL_AIF1_MSTR_MOD), > -value << SUN8I_AIF1CLK_CTRL_AIF1_MSTR_MOD); > + > + regmap_update_bits(scodec->regmap, reg, > +BIT(SUN8I_AIF_CLK_CTRL_MSTR_MOD), > +value << SUN8I_AIF_CLK_CTRL_MSTR_MOD); I guess it would be more readable without the intermediate variable to store the register. With that fixed, Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH 22/25] ASoC: sun8i-codec: Enable all supported PCM formats
On Wed, Sep 30, 2020 at 09:11:45PM -0500, Samuel Holland wrote: > Now that the DAI clock setup is correct for all hardware-supported PCM > formats, we can enable them in the driver. With the appropriate support > in the CPU DAI driver, this allows userspace to access the additional > formats. > > Since this codec is connected to the CPU via a DAI, not directly, we do > not care if the CPU DAI is using 3-byte or 4-byte formats, so we can > support them both. > > Signed-off-by: Samuel Holland Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH 21/25] ASoC: sun8i-codec: Require an exact BCLK divisor match
On Wed, Sep 30, 2020 at 09:11:44PM -0500, Samuel Holland wrote: > Now that we guarantee that SYSCLK is running at the optimal rate when > hw_params succeeds, and that it will continue running at that rate, > SYSCLK will always be an integer multiple of BCLK. So we can always > pick the exact divider, not just the closest divider. > > Signed-off-by: Samuel Holland Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH 20/25] ASoC: sun8i-codec: Protect the clock rate while streams are open
On Wed, Sep 30, 2020 at 09:11:43PM -0500, Samuel Holland wrote:
> The codec's clock input is shared among all AIFs, and shared with other
> audio-related hardware in the SoC, including I2S and SPDIF controllers.
> To ensure sample rates selected by userspace or by codec2codec DAI links
> are maintained, the clock rate must be protected while it is in use.
>
> Signed-off-by: Samuel Holland
> ---
> sound/soc/sunxi/sun8i-codec.c | 25 ++---
> 1 file changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
> index 501af64d43a0..86065bee7cd3 100644
> --- a/sound/soc/sunxi/sun8i-codec.c
> +++ b/sound/soc/sunxi/sun8i-codec.c
> @@ -416,27 +416,32 @@ static int sun8i_codec_get_lrck_div_order(unsigned int
> slots,
> unsigned int div = slots * slot_width;
>
> if (div < 16 || div > 256)
> return -EINVAL;
>
> return order_base_2(div);
> }
>
> +static unsigned int sun8i_codec_get_sysclk_rate(unsigned int sample_rate)
> +{
> + return sample_rate % 4000 ? 22579200 : 24576000;
> +}
> +
> static int sun8i_codec_hw_params(struct snd_pcm_substream *substream,
>struct snd_pcm_hw_params *params,
>struct snd_soc_dai *dai)
> {
> struct sun8i_codec *scodec = snd_soc_dai_get_drvdata(dai);
> struct sun8i_codec_aif *aif = >aifs[dai->id];
> unsigned int sample_rate = params_rate(params);
> unsigned int slots = aif->slots ?: params_channels(params);
> unsigned int slot_width = aif->slot_width ?: params_width(params);
> - unsigned int sysclk_rate = clk_get_rate(scodec->clk_module);
> - int lrck_div_order, word_size;
> + unsigned int sysclk_rate = sun8i_codec_get_sysclk_rate(sample_rate);
> + int lrck_div_order, ret, word_size;
> u8 bclk_div;
>
> /* word size */
> switch (params_width(params)) {
> case 8:
> word_size = 0x0;
> break;
> case 16:
> @@ -466,17 +471,30 @@ static int sun8i_codec_hw_params(struct
> snd_pcm_substream *substream,
> (lrck_div_order - 4) <<
> SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV);
>
> /* BCLK divider (SYSCLK/BCLK ratio) */
> bclk_div = sun8i_codec_get_bclk_div(sysclk_rate, lrck_div_order,
> sample_rate);
> regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
> SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV_MASK,
> bclk_div << SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV);
>
> - if (!aif->open_streams) {
> + /* SYSCLK rate */
> + if (aif->open_streams) {
> + ret = clk_set_rate(scodec->clk_module, sysclk_rate);
> + if (ret < 0)
> + return ret;
> + } else {
> + ret = clk_set_rate_exclusive(scodec->clk_module, sysclk_rate);
It's not really clear to me why we wouldn't want to always protect the
clock rate here?
> + if (ret == -EBUSY)
> + dev_err(dai->dev, "%s: clock is busy! Sample rate %u Hz
> "
> + "conflicts with other audio streams.\n",
This string creates a checkpatch warning.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH 19/25] ASoC: sun8i-codec: Constrain to compatible sample rates
On Wed, Sep 30, 2020 at 09:11:42PM -0500, Samuel Holland wrote: > While another stream is active, only allow userspace to use sample rates > that are compatible with the current SYSCLK frequency. This ensures the > actual sample rate will always match what is given in hw_params. > > Signed-off-by: Samuel Holland Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH 18/25] ASoC: sun8i-codec: Automatically set the system sample rate
On Wed, Sep 30, 2020 at 09:11:41PM -0500, Samuel Holland wrote: > The sun8i codec has three clock/sample rate domains: > - The AIF1 domain, with a sample rate equal to AIF1 LRCK > - The AIF2 domain, with a sample rate equal to AIF2 LRCK > - The SYSCLK domain, containing the ADC, DAC, and effects (AGC/DRC), >with a sample rate given by a divisor from SYSCLK. The divisor is >controlled by the AIF1_FS or AIF2_FS field in SYS_SR_CTRL, depending >on if SYSCLK's source is AIF1CLK or AIF2CLK, respectively. The exact >sample rate depends on if SYSCLK is running at 22.6 MHz or 24.6 MHz. > > When an AIF (currently only AIF1) is active, the ADC and DAC should run > at that sample rate to avoid artifacting. Sample rate conversion is only > available when multiple AIFs are active and are routed to each other; > this means the sample rate conversion hardware usually cannot be used. > > Only attach the event hook to the channel 0 AIF widgets, since we only > need one event when a DAI stream starts or stops. Channel 0 is always > brought up with a DAI stream, regardless of the number of channels in > the stream. > > The ADC and DAC (along with their effects blocks) can be used even if > no AIFs are in use. In that case, we should select an appropriate sample > rate divisor, instead of keeping the last-used AIF sample rate. > 44.1/48 kHz was chosen to balance audio quality and power consumption. > > Since the sample rate is tied to active AIF paths, disabling pmdown_time > allows switching to the optimal sample rate immediately, instead of > after a 5 second delay. > > Signed-off-by: Samuel Holland Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH 17/25] ASoC: sun8i-codec: Enable all supported sample rates
On Wed, Sep 30, 2020 at 09:11:40PM -0500, Samuel Holland wrote: > The system sample rate programmed into the hardware is really a clock > divider from SYSCLK to the ADC and DAC. Since we support two SYSCLK > frequencies, we can use all sample rates corresponding to one of those > frequencies divided by any available divisor. > > This commit enables support for those sample rates. It also stops > advertising support for a 64 kHz sample rate, which is not supported. > > Signed-off-by: Samuel Holland Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH 16/25] ASoC: sun8i-codec: Enforce symmetric DAI parameters
On Wed, Sep 30, 2020 at 09:11:39PM -0500, Samuel Holland wrote: > The AIFs have a single register controlling DAI parameters in both > directions, including BCLK/LRCK divisor word size. The DAIs produce only > noise or silence if any of these parameters is wrong. Therefore, we need > to enforce symmetry for these parameters, so starting a new substream > will not break an existing substream. > > Signed-off-by: Samuel Holland Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH 15/25] ASoC: sun8i-codec: Support the TDM slot binding
On Wed, Sep 30, 2020 at 09:11:38PM -0500, Samuel Holland wrote: > Now that BCLK and LRCK rate calculations can handle any > hardware-supported slot width and number of slots, enable > support for overriding these parameters from the device tree. > > Signed-off-by: Samuel Holland Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH 14/25] ASoC: sun8i-codec: Correct the BCLK divisor calculation
On Wed, Sep 30, 2020 at 09:11:37PM -0500, Samuel Holland wrote: > Previously, the BCLK divisor calculation assumed a power-of-two slot > width and exactly two slots. In order to support the TDM slot binding > and 20/24-bit word sizes, those assumptions must be removed. > > Due to hardware limitations, the BCLK/LRCK ratio is not as simple as > "slot_width * slots". However, the correct value is already calculated > elsewhere in this function, since it must also be programmed into the > hardware. Reuse that value to calculate the correct SYSCLK/BCLK divisor. > > Signed-off-by: Samuel Holland Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH 13/25] ASoC: sun8i-codec: Round up the LRCK divisor
On Wed, Sep 30, 2020 at 09:11:36PM -0500, Samuel Holland wrote: > The codec supports only power-of-two BCLK/LRCK divisors. If either the > slot width or the number of slots is not a power of two, the LRCK > divisor must be rounded up to provide enough space. To do that, use > order_base_2 (instead of ilog2, which rounds down). > > Since the rounded divisor is also needed for setting the SYSCLK/BCLK > divisor, return the order base 2 instead of fully calculating the > hardware register encoding. > > Signed-off-by: Samuel Holland Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH 12/25] ASoC: sun8i-codec: Program the correct word size
On Wed, Sep 30, 2020 at 09:11:35PM -0500, Samuel Holland wrote: > The hardware supports 8 to 24-bit word sizes on all three of its DAIs, > only one of which is connected to the CPU DAI. Program the word size > based on the actual selected format, instead of relying on limitations > in another driver (which, incedentally, has patches pending to remove > that limitation). > > Signed-off-by: Samuel Holland Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH 11/25] ASoC: sun8i-codec: Enable all supported clock inversions
On Wed, Sep 30, 2020 at 09:11:34PM -0500, Samuel Holland wrote:
> When using the I2S, LEFT_J, or RIGHT_J format, the hardware supports
> independent BCLK and LRCK inversion control. When using DSP_A or DSP_B,
> LRCK inversion is not supported. The register bit is repurposed to
> select between DSP_A and DSP_B. Extend the driver to support this.
>
> Signed-off-by: Samuel Holland
> ---
> sound/soc/sunxi/sun8i-codec.c | 57 +++
> 1 file changed, 37 insertions(+), 20 deletions(-)
>
> diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
> index 0b713b2a2028..506420fb355c 100644
> --- a/sound/soc/sunxi/sun8i-codec.c
> +++ b/sound/soc/sunxi/sun8i-codec.c
> @@ -39,18 +39,17 @@
> #define SUN8I_MOD_RST_CTL_AIF1 15
> #define SUN8I_MOD_RST_CTL_ADC3
> #define SUN8I_MOD_RST_CTL_DAC2
> #define SUN8I_SYS_SR_CTRL0x018
> #define SUN8I_SYS_SR_CTRL_AIF1_FS12
> #define SUN8I_SYS_SR_CTRL_AIF2_FS8
> #define SUN8I_AIF1CLK_CTRL 0x040
> #define SUN8I_AIF1CLK_CTRL_AIF1_MSTR_MOD 15
> -#define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_INV 14
> -#define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV 13
> +#define SUN8I_AIF1CLK_CTRL_AIF1_CLK_INV 13
> #define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV 9
> #define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV 6
> #define SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ 4
> #define SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ_16 (1 << 4)
> #define SUN8I_AIF1CLK_CTRL_AIF1_DATA_FMT 2
> #define SUN8I_AIF1_ADCDAT_CTRL 0x044
> #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0L_ENA 15
> #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0R_ENA 14
> @@ -85,16 +84,17 @@
> #define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF1DA1R 10
> #define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF2DACR 9
> #define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_ADCR 8
>
> #define SUN8I_SYSCLK_CTL_AIF1CLK_SRC_MASKGENMASK(9, 8)
> #define SUN8I_SYSCLK_CTL_AIF2CLK_SRC_MASKGENMASK(5, 4)
> #define SUN8I_SYS_SR_CTRL_AIF1_FS_MASK GENMASK(15, 12)
> #define SUN8I_SYS_SR_CTRL_AIF2_FS_MASK GENMASK(11, 8)
> +#define SUN8I_AIF1CLK_CTRL_AIF1_CLK_INV_MASK GENMASK(14, 13)
> #define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV_MASKGENMASK(12, 9)
> #define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV_MASKGENMASK(8, 6)
> #define SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ_MASKGENMASK(5, 4)
> #define SUN8I_AIF1CLK_CTRL_AIF1_DATA_FMT_MASKGENMASK(3, 2)
>
> enum {
> AIF1,
> NAIFS
> @@ -168,17 +168,17 @@ static int sun8i_codec_get_hw_rate(struct
> snd_pcm_hw_params *params)
> default:
> return -EINVAL;
> }
> }
>
> static int sun8i_codec_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> {
> struct sun8i_codec *scodec = snd_soc_dai_get_drvdata(dai);
> - u32 format, value;
> + u32 format, invert, value;
>
> /* clock masters */
> switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> case SND_SOC_DAIFMT_CBS_CFS: /* Codec slave, DAI master */
> value = 0x1;
> break;
> case SND_SOC_DAIFMT_CBM_CFM: /* Codec Master, DAI slave */
> value = 0x0;
> @@ -197,55 +197,72 @@ static int sun8i_codec_set_fmt(struct snd_soc_dai *dai,
> unsigned int fmt)
> break;
> case SND_SOC_DAIFMT_LEFT_J:
> format = 0x1;
> break;
> case SND_SOC_DAIFMT_RIGHT_J:
> format = 0x2;
> break;
> case SND_SOC_DAIFMT_DSP_A:
> + format = 0x3;
> + invert = 0x0; /* Set LRCK_INV to 0 */
> + break;
> case SND_SOC_DAIFMT_DSP_B:
> format = 0x3;
> + invert = 0x1; /* Set LRCK_INV to 1 */
> break;
> default:
> return -EINVAL;
> }
> regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
> SUN8I_AIF1CLK_CTRL_AIF1_DATA_FMT_MASK,
> format << SUN8I_AIF1CLK_CTRL_AIF1_DATA_FMT);
>
> /* clock inversion */
> switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
> case SND_SOC_DAIFMT_NB_NF: /* Normal */
> value = 0x0;
> break;
> - case SND_SOC_DAIFMT_IB_IF: /* Inversion */
> + case SND_SOC_DAIFMT_NB_IF: /* Inverted LRCK */
> value = 0x1;
> break;
> + case SND_SOC_DAIFMT_IB_NF: /* Inverted BCLK */
> + value = 0x2;
> + break;
> + case SND_SOC_DAIFMT_IB_IF: /* Both inverted */
> + value = 0x3;
> + break;
> default:
> return -EINVAL;
> }
> - regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
> -
Re: [PATCH 10/25] ASoC: sun8i-codec: Program format before clock inversion
On Wed, Sep 30, 2020 at 09:11:33PM -0500, Samuel Holland wrote: > The LRCK inversion bit has a different meaning in DSP mode: it selects > between DSP A and DSP B formats. To support this, we need to know if > the selected format is a DSP format. One easy way to do this is to set > the format field before the inversion fields. > > Signed-off-by: Samuel Holland Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH 09/25] ASoC: sun8i-codec: Prepare to extend the DAI driver
Hi,
On Wed, Sep 30, 2020 at 09:11:32PM -0500, Samuel Holland wrote:
> In preparation for adding additional DAIs to this component, convert the
> DAI driver definition to an array. Since this changes all of the lines
> in the definition anyway, let's move it closer to the ops function
> definitions, instead of on the far side of the DAPM arrays. And while
> moving the DAI driver ops, rename the set_fmt hook to match the usual
> naming scheme.
>
> Give the existing DAI an explicit ID and more meaningful stream names,
> so it will remain unique as more DAIs are added. The AIF widget streams
> must be updated to match.
>
> Signed-off-by: Samuel Holland
> ---
> sound/soc/sunxi/sun8i-codec.c | 76 +++
> 1 file changed, 42 insertions(+), 34 deletions(-)
>
> diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
> index 7590c4b04d14..346f699c2e86 100644
> --- a/sound/soc/sunxi/sun8i-codec.c
> +++ b/sound/soc/sunxi/sun8i-codec.c
> @@ -90,16 +90,21 @@
> #define SUN8I_SYSCLK_CTL_AIF2CLK_SRC_MASKGENMASK(5, 4)
> #define SUN8I_SYS_SR_CTRL_AIF1_FS_MASK GENMASK(15, 12)
> #define SUN8I_SYS_SR_CTRL_AIF2_FS_MASK GENMASK(11, 8)
> #define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV_MASKGENMASK(12, 9)
> #define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV_MASKGENMASK(8, 6)
> #define SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ_MASKGENMASK(5, 4)
> #define SUN8I_AIF1CLK_CTRL_AIF1_DATA_FMT_MASKGENMASK(3, 2)
>
> +enum {
> + AIF1,
> + NAIFS
> +};
> +
It's a bit of a nitpick, but we should have less generic names for the
enums here, maybe prefix it with sun8i_codec like the rest of the driver?
Once fixed,
Acked-by: Maxime Ripard
Maxime
signature.asc
Description: PGP signature
