Re: [linux-sunxi] [PATCH] ASoC: sun4i-i2s: Incorrect SR and WSS computation
On Mon, Jul 29, 2019 at 8:21 AM wrote:
>
> From: Marcus Cooper
>
> The A64 audio codec uses the original I2S block but the SR and
> WSS computation currently assigned is for the newer block.
>
> Fixes: 619c15f7fac9 (ASoC: sun4i-i2s: Change SR and WSS computation)
Looks like we need this fix for 5.3.
> Signed-off-by: Marcus Cooper
Reviewed-by: Vasily Khoruzhick
> ---
> sound/soc/sunxi/sun4i-i2s.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index 9b2232908b65..7fa5c61169db 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -1002,8 +1002,8 @@ static const struct sun4i_i2s_quirks
> sun50i_a64_codec_i2s_quirks = {
> .field_rxchanmap= REG_FIELD(SUN4I_I2S_RX_CHAN_MAP_REG, 0, 31),
> .field_txchansel= REG_FIELD(SUN4I_I2S_TX_CHAN_SEL_REG, 0, 2),
> .field_rxchansel= REG_FIELD(SUN4I_I2S_RX_CHAN_SEL_REG, 0, 2),
> - .get_sr = sun8i_i2s_get_sr_wss,
> - .get_wss= sun8i_i2s_get_sr_wss,
> + .get_sr = sun4i_i2s_get_sr,
> + .get_wss= sun4i_i2s_get_wss,
> };
>
> static int sun4i_i2s_init_regmap_fields(struct device *dev,
> --
> 2.22.0
>
> --
> You received this message because you are subscribed to the Google Groups
> "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to linux-sunxi+unsubscr...@googlegroups.com.
> To view this discussion on the web, visit
> https://groups.google.com/d/msgid/linux-sunxi/20190729152130.27955-1-codekipper%40gmail.com.
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
To view this discussion on the web, visit
https://groups.google.com/d/msgid/linux-sunxi/CA%2BE%3DqVd3rd0qxY%2BeV%3DzTnPy1e9zeVbthMn3ieLqCkbTwEPJFYA%40mail.gmail.com.
Re: [linux-sunxi] Re: thermal zones on R40
Clément,
I gave it a try but still something missing.
cat /sys/class/thermal/thermal_zone0/temp
22506
cat /sys/class/thermal/thermal_zone0/temp
22471
cat /sys/class/thermal/thermal_zone0/temp
22471
cat /sys/class/thermal/thermal_zone0/temp
22480
cat /sys/class/thermal/thermal_zone0/temp
22471
cat /sys/class/thermal/thermal_zone0/temp
22462
#define MULPA (u32)(0.1125*1024*1024)
#define DIVPA (20)
#define MINUPA (250*1024*1024)
static int sun8i_ths_get_temp(void *_data, int *out)
{
struct sun8i_ths_data *data = _data;
s32 reg;
if (data->temp == 0)
return -EBUSY;
/* Formula and parameters from the Allwinner BSP 4.4 kernel */
reg = (MINUPA - (data->temp << DIVPA));
reg = reg / MULPA;
*out = reg;
return 0;
}
ths: ths@1c24c00 {
#thermal-sensor-cells = <0>;
compatible = "allwinner,sun8i-h3-ths";
reg = <0x1c24c00 0x400>,
<0x01c1b234 0x4>;
reg-names = "ths", "calibration";
interrupts = ;
resets = <&ccu RST_BUS_THS>;
reset-names = "ahb";
clocks = <&ccu CLK_BUS_THS>, <&ccu CLK_THS>;
clock-names = "ahb", "ths";
};
Regards,
@lex
On Sunday, July 28, 2019 at 5:44:31 PM UTC-3, Clément Péron wrote:
>
> On Sun, 28 Jul 2019 at 22:21, Clément Péron > wrote:
> >
> > Hi Alex,
> >
> > On Thu, 25 Jul 2019 at 23:20, @lex >
> wrote:
> > >
> > > Hi Clément,
> > >
> > > I am using the H3 THS driver, maybe this needs a rewrite.
> > > 3.15.3.2. Temperature Conversion Formula TBD. Could be just that, but
> who knows.
> > > At least i tried.
> >
> > Yes maybe the formula is different but I just check for the
> > calibration data and my previous message was wrong.
> > The calibration data for the H3 are pointing to the SID (internal
> eeprom).
> >
> > H3 => 0x01C1 4000
> > R40 => 0x01C1 B000
> >
> > So maybe you can try with this bindings.
> >
> > reg = <0x1c24c00 0x400>,
> > <0x01c1b234 0x4>;
>
> You have to check this offset and the formula from the vendor kernel.
>
> For the calibration data this seems to be "0x01c1b240" :
>
> https://github.com/BPI-SINOVOIP/BPI-M2U-bsp/blob/cbfb10bffba398dd109262af9704fe70cb4b2f3e/linux-sunxi/drivers/soc/allwinner/sunxi-sid-efuse.h#L49
>
>
> For the formula you have to check here:
>
> https://github.com/BPI-SINOVOIP/BPI-M2U-bsp/blob/cbfb10bffba398dd109262af9704fe70cb4b2f3e/linux-sunxi/drivers/thermal/sunxi_thermal/sun8iw11_ths.c#L65
>
>
> And you can check a bit the dt bindings for IRQ and memory mapping here :
>
> https://github.com/BPI-SINOVOIP/BPI-M2U-bsp/blob/master/linux-sunxi/arch/arm/boot/dts/sun8iw11p1.dtsi#L1601-L1607
>
>
>
> >
> > Hope this can help.
> >
> > Regards,
> > Clément
> >
> > >
> > > Thank you anyway.
> > >
> > > On Thursday, July 25, 2019 at 5:37:39 PM UTC-3, Clément Péron wrote:
> > >>
> > >> Hi Alex,
> > >>
> > >> On Thu, 25 Jul 2019 at 22:01, @lex wrote:
> > >> >
> > >> > Thank you for the info.
> > >> >
> > >> > Almost there, i get negative values for the temp, still something
> missing or wrong.
> > >>
> > >> This mean that the calibration data are wrong.
> > >>
> > >> I don't know where the H3 is looking for his callibration data.
> > >>
> > >> But the R40 seems to have a register for that with an offset of 0x74:
> > >> Maybe you should try :
> > >> reg = <0x1c24c00 0x400>,
> > >><0x1c24c74 0x4>;
> > >>
> > >> Else you have to open the H3 user manual and understand what this
> > >> "0x01c14234" address is pointing to.
> > >> And look for the same information for R40.
> > >>
> > >> Regards,
> > >> Clément
> > >>
> > >> >
> > >> > I came up with this:
> > >> >
> > >> >> ths: ths@1c24c00 {
> > >> >> #thermal-sensor-cells = <0>;
> > >> >> compatible = "allwinner,sun8i-h3-ths";
> > >> >> reg = <0x1c24c00 0x400>,
> > >> >> <0x01c14234 0x4>;
> > >> >> reg-names = "ths", "calibration";
> > >> >> interrupts = ;
> > >> >> resets = <&ccu RST_BUS_THS>;
> > >> >> reset-names = "ahb";
> > >> >> clocks = <&ccu CLK_BUS_THS>, <&ccu CLK_THS>;
> > >> >> clock-names = "ahb", "ths";
> > >> >> };
> > >> >
> > >> >
> > >> > On Thursday, July 25, 2019 at 4:28:07 PM UTC-3, Clément Péron
> wrote:
> > >> >>
> > >> >> Hi Alex,
> > >> >>
> > >> >> On Thu, 25 Jul 2019 at 19:12, @lex wrote:
> > >> >> >
> > >> >> > Hi Clément,
> > >> >> >
> > >> >> > No. i did not read the R40 manual until now, Base address:
> 1c24c00
> > >> >> > But how to find the correct interrupt number and the correct
> Mapping in the Manual? What should o look for?
> > >> >> You have to look for GIC interrupt table (page 205).
> > >> >> And you have to find the correct interrupt number.
> > >> >> To have it you have to subtract 32 (the first 32 interrupts are
> ARM reserved).
> > >> >>
> > >> >> Regards,
Re: [linux-sunxi] Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM
Dne ponedeljek, 29. julij 2019 ob 20:51:08 CEST je Uwe Kleine-König
napisal(a):
> On Mon, Jul 29, 2019 at 08:46:25PM +0200, Jernej Škrabec wrote:
> > Dne ponedeljek, 29. julij 2019 ob 20:40:41 CEST je Uwe Kleine-König
> >
> > napisal(a):
> > > On Mon, Jul 29, 2019 at 06:40:15PM +0200, Jernej Škrabec wrote:
> > > > Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König
> > > >
> > > > napisal(a):
> > > > > Hello,
> > > > >
> > > > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> > > > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> > > > > >
> > > > > > wrote:
> > > > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe
> > > > > > > > Kleine-König
> > > > > > > >
> > > > > > > > napisal(a):
> > > > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec
wrote:
> > > > > > > > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > > > > > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > > > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > > > > > > > > > sun4i_pwm_single_bypass = {>
> > > > > > > > > >
> > > > > > > > > > .npwm = 1,
> > > > > > > > > >
> > > > > > > > > > };
> > > > > > > > > >
> > > > > > > > > > +static const struct sun4i_pwm_data
> > > > > > > > > > sun50i_pwm_dual_bypass_clk_rst
> > > > > > > > > > = {
> > > > > > > > > > + .has_bus_clock = true,
> > > > > > > > > > + .has_prescaler_bypass = true,
> > > > > > > > > > + .has_reset = true,
> > > > > > > > > > + .npwm = 2,
> > > > > > > > > > +};
> > > > > > > > > > +
> > > > > > > > > >
> > > > > > > > > > static const struct of_device_id sun4i_pwm_dt_ids[] = {
> > > > > > > > > >
> > > > > > > > > > {
> > > > > > > > > >
> > > > > > > > > > .compatible = "allwinner,sun4i-a10-pwm",
> > > > > > > > > >
> > > > > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id
> > > > > > > > > > sun4i_pwm_dt_ids[] =
> > > > > > > > > > {
> > > > > > > > > >
> > > > > > > > > > }, {
> > > > > > > > > >
> > > > > > > > > > .compatible = "allwinner,sun8i-h3-pwm",
> > > > > > > > > > .data = &sun4i_pwm_single_bypass,
> > > > > > > > > >
> > > > > > > > > > + }, {
> > > > > > > > > > + .compatible = "allwinner,sun50i-h6-pwm",
> > > > > > > > > > + .data = &sun50i_pwm_dual_bypass_clk_rst,
> > > > > > > > >
> > > > > > > > > If you follow my suggestion for the two previous patches,
> > > > > > > > > you
> > > > > > > > > can
> > > > > > > > > just
> > > > > > > > >
> > > > > > > > > use:
> > > > > > > > > compatible = "allwinner,sun50i-h6-pwm",
> > > > > > > > > "allwinner,sun5i-a10s-pwm";
> > > > > > > > >
> > > > > > > > > and drop this patch.
> > > > > > > >
> > > > > > > > Maxime found out that it's not compatible with A10s due to
> > > > > > > > difference
> > > > > > > > in bypass bit, but yes, I know what you mean.
> > > > > > > >
> > > > > > > > Since H6 requires reset line and bus clock to be specified,
> > > > > > > > it's
> > > > > > > > not
> > > > > > > > compatible from DT binding side. New yaml based binding must
> > > > > > > > somehow
> > > > > > > > know that in order to be able to validate DT node, so it needs
> > > > > > > > standalone compatible. However, depending on conclusions of
> > > > > > > > other
> > > > > > > > discussions, this new compatible can be associated with
> > > > > > > > already
> > > > > > > > available quirks structure or have it's own.> >
> > > > > > >
> > > > > > > I cannot follow. You should be able to specify in the binding
> > > > > > > that
> > > > > > > the
> > > > > > > reset line and bus clock is optional. Then
> > > > > > > allwinner,sun50i-h6-pwm
> > > > > > > without a reset line and bus clock also verifies, but this
> > > > > > > doesn't
> > > > > > > really hurt (and who knows, maybe the next allwinner chip needs
> > > > > > > exactly
> > > > > > > this).
> > > > > >
> > > > > > It is not optional. It will not work if either the clocks or reset
> > > > > > controls
> > > > > > are missing. How would these be optional anyway? Either it's
> > > > > > connected
> > > > > > and
> > > > > > thus required, or it's not and therefore should be omitted from
> > > > > > the
> > > > > > description.
> > > > >
> > > > > [Just arguing about the clock here, the argumentation is analogous
> > > > > for
> > > > > the reset control.]
> > > > >
> > > > > From the driver's perspective it's optional: There are devices with
> > > > > and
> > > > > without a bus clock. This doesn't mean that you can just ignore this
> > > > > clock if it's specified. It's optional in the sense "If dt doesn't
> > > > > specify it, then assume this is a device that doesn't have it and so
> > > > > you
> > > > > don't need to handle it." but not in the sense "it doesn't matter if
> > > > > you handle it or not.".
> > > > >
> > > > > Other than that I'm on your side. So for example I think it's not
> > > >
[linux-sunxi] Re: [PATCH 3/6] pwm: sun4i: Add a quirk for bus clock
Hello Maxime,
On Mon, Jul 29, 2019 at 06:45:16PM +0200, Maxime Ripard wrote:
> On Mon, Jul 29, 2019 at 06:14:35PM +0200, Uwe Kleine-König wrote:
> > Then maybe something like the following?:
> >
> > busclk = devm_clk_get_optional(..., "bus");
> > modclk = devm_clk_get_optional(..., "mod");
> >
> > /*
> > * old dtbs might have a single clock but no clock names. Fall
> > * back to this for compatibility reasons.
> > */
> > if (!modclk) {
> > modclk = devm_clk_get(..., NULL);
> > }
>
> Again, there's nothing optional about these clocks. You need a
> particular set of clocks for a given generation, and a separate set of
> them on another generation of SoCs.
It depends on the way how "optional" is understood. The semantic of
"optional" as it is used and implemented by devm_clk_get_optional (and
gpiod_get_optional and devm_reset_control_get_optional) is different
than yours when saying "on H6 the clock is not optional". If it was
about the "it doesn't matter if it's taken care of or not" semantic you
seem to mean the function would be useless and no driver would need to
actually use it. In the sense of the functions listed above "optional"
means: Some devices need it, others don't. Using this semantic the "bus"
clock is optional.
> It really isn't about DT validation. We're really making sure that the
> device can be operational. It's as much of a validation step than
> making sure we have mapped registers (reg), or an interrupt if we had
> any.
Do you agree with Jernej in the other end of this thread? If so I don't
think that repeating the same arguments here is sensible. Please read
what I wrote there.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
To view this discussion on the web, visit
https://groups.google.com/d/msgid/linux-sunxi/20190729190404.rnxw2aihmciafy6c%40pengutronix.de.
Re: [linux-sunxi] Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM
On Mon, Jul 29, 2019 at 08:46:25PM +0200, Jernej Škrabec wrote:
> Dne ponedeljek, 29. julij 2019 ob 20:40:41 CEST je Uwe Kleine-König
> napisal(a):
> > On Mon, Jul 29, 2019 at 06:40:15PM +0200, Jernej Škrabec wrote:
> > > Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König
> > >
> > > napisal(a):
> > > > Hello,
> > > >
> > > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> > > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> > > > >
> > > > > wrote:
> > > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe
> > > > > > > Kleine-König
> > > > > > >
> > > > > > > napisal(a):
> > > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > > > > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > > > > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > > > > > > > > sun4i_pwm_single_bypass = {>
> > > > > > > > >
> > > > > > > > > .npwm = 1,
> > > > > > > > >
> > > > > > > > > };
> > > > > > > > >
> > > > > > > > > +static const struct sun4i_pwm_data
> > > > > > > > > sun50i_pwm_dual_bypass_clk_rst
> > > > > > > > > = {
> > > > > > > > > + .has_bus_clock = true,
> > > > > > > > > + .has_prescaler_bypass = true,
> > > > > > > > > + .has_reset = true,
> > > > > > > > > + .npwm = 2,
> > > > > > > > > +};
> > > > > > > > > +
> > > > > > > > >
> > > > > > > > > static const struct of_device_id sun4i_pwm_dt_ids[] = {
> > > > > > > > >
> > > > > > > > > {
> > > > > > > > >
> > > > > > > > > .compatible = "allwinner,sun4i-a10-pwm",
> > > > > > > > >
> > > > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id
> > > > > > > > > sun4i_pwm_dt_ids[] =
> > > > > > > > > {
> > > > > > > > >
> > > > > > > > > }, {
> > > > > > > > >
> > > > > > > > > .compatible = "allwinner,sun8i-h3-pwm",
> > > > > > > > > .data = &sun4i_pwm_single_bypass,
> > > > > > > > >
> > > > > > > > > + }, {
> > > > > > > > > + .compatible = "allwinner,sun50i-h6-pwm",
> > > > > > > > > + .data = &sun50i_pwm_dual_bypass_clk_rst,
> > > > > > > >
> > > > > > > > If you follow my suggestion for the two previous patches, you
> > > > > > > > can
> > > > > > > > just
> > > > > > > >
> > > > > > > > use:
> > > > > > > > compatible = "allwinner,sun50i-h6-pwm",
> > > > > > > > "allwinner,sun5i-a10s-pwm";
> > > > > > > >
> > > > > > > > and drop this patch.
> > > > > > >
> > > > > > > Maxime found out that it's not compatible with A10s due to
> > > > > > > difference
> > > > > > > in bypass bit, but yes, I know what you mean.
> > > > > > >
> > > > > > > Since H6 requires reset line and bus clock to be specified, it's
> > > > > > > not
> > > > > > > compatible from DT binding side. New yaml based binding must
> > > > > > > somehow
> > > > > > > know that in order to be able to validate DT node, so it needs
> > > > > > > standalone compatible. However, depending on conclusions of other
> > > > > > > discussions, this new compatible can be associated with already
> > > > > > > available quirks structure or have it's own.> >
> > > > > >
> > > > > > I cannot follow. You should be able to specify in the binding that
> > > > > > the
> > > > > > reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm
> > > > > > without a reset line and bus clock also verifies, but this doesn't
> > > > > > really hurt (and who knows, maybe the next allwinner chip needs
> > > > > > exactly
> > > > > > this).
> > > > >
> > > > > It is not optional. It will not work if either the clocks or reset
> > > > > controls
> > > > > are missing. How would these be optional anyway? Either it's connected
> > > > > and
> > > > > thus required, or it's not and therefore should be omitted from the
> > > > > description.
> > > >
> > > > [Just arguing about the clock here, the argumentation is analogous for
> > > > the reset control.]
> > > >
> > > > From the driver's perspective it's optional: There are devices with and
> > > > without a bus clock. This doesn't mean that you can just ignore this
> > > > clock if it's specified. It's optional in the sense "If dt doesn't
> > > > specify it, then assume this is a device that doesn't have it and so you
> > > > don't need to handle it." but not in the sense "it doesn't matter if
> > > > you handle it or not.".
> > > >
> > > > Other than that I'm on your side. So for example I think it's not
> > > > optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that
> > > > devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n
> > > > because this hides exactly the kind of problem you point out here.
> > >
> > > I think there's misunderstanding. I only argued that we can't use
> > >
> > > compatible = "allwinner,sun50i-h6-pwm",
> > >
> > >"allwinner,sun5i-a10s-pwm";
> > >
> > > as you suggested and only
> > >
> > > compatible = "al
Re: [linux-sunxi] Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM
Dne ponedeljek, 29. julij 2019 ob 20:40:41 CEST je Uwe Kleine-König
napisal(a):
> On Mon, Jul 29, 2019 at 06:40:15PM +0200, Jernej Škrabec wrote:
> > Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König
> >
> > napisal(a):
> > > Hello,
> > >
> > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> > > >
> > > > wrote:
> > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe
> > > > > > Kleine-König
> > > > > >
> > > > > > napisal(a):
> > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > > > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > > > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > > > > > > > sun4i_pwm_single_bypass = {>
> > > > > > > >
> > > > > > > > .npwm = 1,
> > > > > > > >
> > > > > > > > };
> > > > > > > >
> > > > > > > > +static const struct sun4i_pwm_data
> > > > > > > > sun50i_pwm_dual_bypass_clk_rst
> > > > > > > > = {
> > > > > > > > + .has_bus_clock = true,
> > > > > > > > + .has_prescaler_bypass = true,
> > > > > > > > + .has_reset = true,
> > > > > > > > + .npwm = 2,
> > > > > > > > +};
> > > > > > > > +
> > > > > > > >
> > > > > > > > static const struct of_device_id sun4i_pwm_dt_ids[] = {
> > > > > > > >
> > > > > > > > {
> > > > > > > >
> > > > > > > > .compatible = "allwinner,sun4i-a10-pwm",
> > > > > > > >
> > > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id
> > > > > > > > sun4i_pwm_dt_ids[] =
> > > > > > > > {
> > > > > > > >
> > > > > > > > }, {
> > > > > > > >
> > > > > > > > .compatible = "allwinner,sun8i-h3-pwm",
> > > > > > > > .data = &sun4i_pwm_single_bypass,
> > > > > > > >
> > > > > > > > + }, {
> > > > > > > > + .compatible = "allwinner,sun50i-h6-pwm",
> > > > > > > > + .data = &sun50i_pwm_dual_bypass_clk_rst,
> > > > > > >
> > > > > > > If you follow my suggestion for the two previous patches, you
> > > > > > > can
> > > > > > > just
> > > > > > >
> > > > > > > use:
> > > > > > > compatible = "allwinner,sun50i-h6-pwm",
> > > > > > > "allwinner,sun5i-a10s-pwm";
> > > > > > >
> > > > > > > and drop this patch.
> > > > > >
> > > > > > Maxime found out that it's not compatible with A10s due to
> > > > > > difference
> > > > > > in bypass bit, but yes, I know what you mean.
> > > > > >
> > > > > > Since H6 requires reset line and bus clock to be specified, it's
> > > > > > not
> > > > > > compatible from DT binding side. New yaml based binding must
> > > > > > somehow
> > > > > > know that in order to be able to validate DT node, so it needs
> > > > > > standalone compatible. However, depending on conclusions of other
> > > > > > discussions, this new compatible can be associated with already
> > > > > > available quirks structure or have it's own.> >
> > > > >
> > > > > I cannot follow. You should be able to specify in the binding that
> > > > > the
> > > > > reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm
> > > > > without a reset line and bus clock also verifies, but this doesn't
> > > > > really hurt (and who knows, maybe the next allwinner chip needs
> > > > > exactly
> > > > > this).
> > > >
> > > > It is not optional. It will not work if either the clocks or reset
> > > > controls
> > > > are missing. How would these be optional anyway? Either it's connected
> > > > and
> > > > thus required, or it's not and therefore should be omitted from the
> > > > description.
> > >
> > > [Just arguing about the clock here, the argumentation is analogous for
> > > the reset control.]
> > >
> > > From the driver's perspective it's optional: There are devices with and
> > > without a bus clock. This doesn't mean that you can just ignore this
> > > clock if it's specified. It's optional in the sense "If dt doesn't
> > > specify it, then assume this is a device that doesn't have it and so you
> > > don't need to handle it." but not in the sense "it doesn't matter if
> > > you handle it or not.".
> > >
> > > Other than that I'm on your side. So for example I think it's not
> > > optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that
> > > devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n
> > > because this hides exactly the kind of problem you point out here.
> >
> > I think there's misunderstanding. I only argued that we can't use
> >
> > compatible = "allwinner,sun50i-h6-pwm",
> >
> > "allwinner,sun5i-a10s-pwm";
> >
> > as you suggested and only
> >
> > compatible = "allwinner,sun50i-h6-pwm";
> >
> > will work. Not because of driver itself (it can still use _optional()
> > variants), but because of DT binding, which should be able to validate H6
> > PWM node - reset and bus clock references are required in this case.
>
> I think I understood. In my eyes there
Re: [linux-sunxi] Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM
On Mon, Jul 29, 2019 at 06:40:15PM +0200, Jernej Škrabec wrote:
> Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König
> napisal(a):
> > Hello,
> >
> > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> > >
> > > wrote:
> > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König
> > > > >
> > > > > napisal(a):
> > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > > > > > > sun4i_pwm_single_bypass = {>
> > > > > > >
> > > > > > > .npwm = 1,
> > > > > > >
> > > > > > > };
> > > > > > >
> > > > > > > +static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst
> > > > > > > = {
> > > > > > > + .has_bus_clock = true,
> > > > > > > + .has_prescaler_bypass = true,
> > > > > > > + .has_reset = true,
> > > > > > > + .npwm = 2,
> > > > > > > +};
> > > > > > > +
> > > > > > >
> > > > > > > static const struct of_device_id sun4i_pwm_dt_ids[] = {
> > > > > > >
> > > > > > > {
> > > > > > >
> > > > > > > .compatible = "allwinner,sun4i-a10-pwm",
> > > > > > >
> > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id
> > > > > > > sun4i_pwm_dt_ids[] =
> > > > > > > {
> > > > > > >
> > > > > > > }, {
> > > > > > >
> > > > > > > .compatible = "allwinner,sun8i-h3-pwm",
> > > > > > > .data = &sun4i_pwm_single_bypass,
> > > > > > >
> > > > > > > + }, {
> > > > > > > + .compatible = "allwinner,sun50i-h6-pwm",
> > > > > > > + .data = &sun50i_pwm_dual_bypass_clk_rst,
> > > > > >
> > > > > > If you follow my suggestion for the two previous patches, you can
> > > > > > just
> > > > > >
> > > > > > use:
> > > > > > compatible = "allwinner,sun50i-h6-pwm",
> > > > > > "allwinner,sun5i-a10s-pwm";
> > > > > >
> > > > > > and drop this patch.
> > > > >
> > > > > Maxime found out that it's not compatible with A10s due to difference
> > > > > in bypass bit, but yes, I know what you mean.
> > > > >
> > > > > Since H6 requires reset line and bus clock to be specified, it's not
> > > > > compatible from DT binding side. New yaml based binding must somehow
> > > > > know that in order to be able to validate DT node, so it needs
> > > > > standalone compatible. However, depending on conclusions of other
> > > > > discussions, this new compatible can be associated with already
> > > > > available quirks structure or have it's own.> >
> > > > I cannot follow. You should be able to specify in the binding that the
> > > > reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm
> > > > without a reset line and bus clock also verifies, but this doesn't
> > > > really hurt (and who knows, maybe the next allwinner chip needs exactly
> > > > this).
> > >
> > > It is not optional. It will not work if either the clocks or reset
> > > controls
> > > are missing. How would these be optional anyway? Either it's connected and
> > > thus required, or it's not and therefore should be omitted from the
> > > description.
> >
> > [Just arguing about the clock here, the argumentation is analogous for
> > the reset control.]
> >
> > From the driver's perspective it's optional: There are devices with and
> > without a bus clock. This doesn't mean that you can just ignore this
> > clock if it's specified. It's optional in the sense "If dt doesn't
> > specify it, then assume this is a device that doesn't have it and so you
> > don't need to handle it." but not in the sense "it doesn't matter if
> > you handle it or not.".
> >
> > Other than that I'm on your side. So for example I think it's not
> > optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that
> > devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n
> > because this hides exactly the kind of problem you point out here.
> >
>
> I think there's misunderstanding. I only argued that we can't use
>
> compatible = "allwinner,sun50i-h6-pwm",
>"allwinner,sun5i-a10s-pwm";
>
> as you suggested and only
>
> compatible = "allwinner,sun50i-h6-pwm";
>
> will work. Not because of driver itself (it can still use _optional()
> variants), but because of DT binding, which should be able to validate H6 PWM
> node - reset and bus clock references are required in this case.
I think I understood. In my eyes there is no need to let validation of
the DT bindings catch a missing "optional" property that is needed on
H6.
You have to draw the line somewhere which information the driver has
hard-coded and what is only provided by the device tree and just assumed
to be correct by the driver. You argue the driver should know that if it
cares for a "allwinner,sun50i-h6-pwm" device it should know (and check)
that there is a clock
[linux-sunxi] Re: [PATCH 2/6] pwm: sun4i: Add a quirk for reset line
On Mon, Jul 29, 2019 at 06:37:15PM +0200, Maxime Ripard wrote:
> On Mon, Jul 29, 2019 at 09:12:18AM +0200, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Mon, Jul 29, 2019 at 02:43:23PM +0800, Chen-Yu Tsai wrote:
> > > On Mon, Jul 29, 2019 at 2:36 PM Uwe Kleine-König
> > > wrote:
> > > > On Fri, Jul 26, 2019 at 08:40:41PM +0200, Jernej Skrabec wrote:
> > > > > @@ -371,6 +374,14 @@ static int sun4i_pwm_probe(struct
> > > > > platform_device *pdev)
> > > > > if (IS_ERR(pwm->clk))
> > > > > return PTR_ERR(pwm->clk);
> > > > >
> > > > > + if (pwm->data->has_reset) {
> > > > > + pwm->rst = devm_reset_control_get(&pdev->dev, NULL);
> > > > > + if (IS_ERR(pwm->rst))
> > > > > + return PTR_ERR(pwm->rst);
> > > > > +
> > > > > + reset_control_deassert(pwm->rst);
> > > > > + }
> > > > > +
> > > >
> > > > I wonder why there is a need to track if a given chip needs a reset
> > > > line. I'd just use devm_reset_control_get_optional() and drop the
> > > > .has_reset member in struct sun4i_pwm_data.
> > >
> > > Because it's not optional for this platform, i.e. it won't work if
> > > the reset control (or clk, in the next patch) is somehow missing from
> > > the device tree.
> >
> > If the device tree is wrong it is considered ok that the driver doesn't
> > behave correctly. So this is not a problem you need (or should) care
> > about.
>
> To some extent that's true, but if we can make the life easier for
> everyone by reporting an error and bailing out instead of silently
> ignoring that, continuing to probe and just ending up with a
> completely broken system for no apparent reason, then why not just do
> that?
>
> I mean, all it takes is three lines of code.
Actually it's more because you need to track for each variant
if it needs the clock (or reset stuff) or not.
It's a weighing between "simpler driver" and "catch unlikely errors".
(If the SoC's .dtsi includes the needed stuff, an error here is really
unlikely, isn't it?)
So to some degree it's subjective which one is better. I tend to prefer
"simpler driver". Also when catching this type of error you have to do
it right twice (in the .dtsi and the driver) while with my approach
there is only a single place that defines what is right, which is a good
design according to what I learned during my studies.
> It's no different than just calling clk_get, and testing the return
> code to see if it's there or not. I wouldn't call that check when you
> depend on a clock "validating the DT". It's just making sure that all
> the resources needed for you to operate properly are there, which is a
> pretty common thing to do.
This is different. As a driver author you are allowed (or even supposed)
to assume that the device tree (or ACPI or platform data ...) is right
and completely defines the stuff for the driven hardware to work
correctly. You must not assume that clk_get() succeeds unconditionally.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
To view this discussion on the web, visit
https://groups.google.com/d/msgid/linux-sunxi/20190729182046.6bwrterbxoceulrx%40pengutronix.de.
Re: [linux-sunxi] Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM
Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König
napisal(a):
> Hello,
>
> On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> >
> > wrote:
> > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König
> > > >
> > > > napisal(a):
> > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > > > > > sun4i_pwm_single_bypass = {>
> > > > > >
> > > > > > .npwm = 1,
> > > > > >
> > > > > > };
> > > > > >
> > > > > > +static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst
> > > > > > = {
> > > > > > + .has_bus_clock = true,
> > > > > > + .has_prescaler_bypass = true,
> > > > > > + .has_reset = true,
> > > > > > + .npwm = 2,
> > > > > > +};
> > > > > > +
> > > > > >
> > > > > > static const struct of_device_id sun4i_pwm_dt_ids[] = {
> > > > > >
> > > > > > {
> > > > > >
> > > > > > .compatible = "allwinner,sun4i-a10-pwm",
> > > > > >
> > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id
> > > > > > sun4i_pwm_dt_ids[] =
> > > > > > {
> > > > > >
> > > > > > }, {
> > > > > >
> > > > > > .compatible = "allwinner,sun8i-h3-pwm",
> > > > > > .data = &sun4i_pwm_single_bypass,
> > > > > >
> > > > > > + }, {
> > > > > > + .compatible = "allwinner,sun50i-h6-pwm",
> > > > > > + .data = &sun50i_pwm_dual_bypass_clk_rst,
> > > > >
> > > > > If you follow my suggestion for the two previous patches, you can
> > > > > just
> > > > >
> > > > > use:
> > > > > compatible = "allwinner,sun50i-h6-pwm",
> > > > > "allwinner,sun5i-a10s-pwm";
> > > > >
> > > > > and drop this patch.
> > > >
> > > > Maxime found out that it's not compatible with A10s due to difference
> > > > in bypass bit, but yes, I know what you mean.
> > > >
> > > > Since H6 requires reset line and bus clock to be specified, it's not
> > > > compatible from DT binding side. New yaml based binding must somehow
> > > > know that in order to be able to validate DT node, so it needs
> > > > standalone compatible. However, depending on conclusions of other
> > > > discussions, this new compatible can be associated with already
> > > > available quirks structure or have it's own.> >
> > > I cannot follow. You should be able to specify in the binding that the
> > > reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm
> > > without a reset line and bus clock also verifies, but this doesn't
> > > really hurt (and who knows, maybe the next allwinner chip needs exactly
> > > this).
> >
> > It is not optional. It will not work if either the clocks or reset
> > controls
> > are missing. How would these be optional anyway? Either it's connected and
> > thus required, or it's not and therefore should be omitted from the
> > description.
>
> [Just arguing about the clock here, the argumentation is analogous for
> the reset control.]
>
> From the driver's perspective it's optional: There are devices with and
> without a bus clock. This doesn't mean that you can just ignore this
> clock if it's specified. It's optional in the sense "If dt doesn't
> specify it, then assume this is a device that doesn't have it and so you
> don't need to handle it." but not in the sense "it doesn't matter if
> you handle it or not.".
>
> Other than that I'm on your side. So for example I think it's not
> optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that
> devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n
> because this hides exactly the kind of problem you point out here.
>
I think there's misunderstanding. I only argued that we can't use
compatible = "allwinner,sun50i-h6-pwm",
"allwinner,sun5i-a10s-pwm";
as you suggested and only
compatible = "allwinner,sun50i-h6-pwm";
will work. Not because of driver itself (it can still use _optional()
variants), but because of DT binding, which should be able to validate H6 PWM
node - reset and bus clock references are required in this case.
Best regards,
Jernej
> Best regards
> Uwe
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
To view this discussion on the web, visit
https://groups.google.com/d/msgid/linux-sunxi/2346193.MplWYqIveT%40jernej-laptop.
[linux-sunxi] Re: [PATCH 5/6] pwm: sun4i: Add support to output source clock directly
Hello,
On Mon, Jul 29, 2019 at 06:16:55PM +0200, Jernej Škrabec wrote:
> Dne ponedeljek, 29. julij 2019 ob 09:06:05 CEST je Uwe Kleine-König
> napisal(a):
> > On Fri, Jul 26, 2019 at 08:40:44PM +0200, Jernej Skrabec wrote:
> > > PWM core has an option to bypass whole logic and output unchanged source
> > > clock as PWM output. This is achieved by enabling bypass bit.
> > >
> > > Note that when bypass is enabled, no other setting has any meaning, not
> > > even enable bit.
> > >
> > > This mode of operation is needed to achieve high enough frequency to
> > > serve as clock source for AC200 chip, which is integrated into same
> > > package as H6 SoC.
> > >
> > > Signed-off-by: Jernej Skrabec
> > > ---
> > >
> > > drivers/pwm/pwm-sun4i.c | 31 ++-
> > > 1 file changed, 30 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > index 9e0eca79ff88..848cff26f385 100644
> > > --- a/drivers/pwm/pwm-sun4i.c
> > > +++ b/drivers/pwm/pwm-sun4i.c
> > > @@ -120,6 +120,19 @@ static void sun4i_pwm_get_state(struct pwm_chip
> > > *chip,
> > >
> > > val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> > >
> > > + /*
> > > + * PWM chapter in H6 manual has a diagram which explains that if bypass
> > > + * bit is set, no other setting has any meaning. Even more, experiment
> > > + * proved that also enable bit is ignored in this case.
> > > + */
> > > + if (val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) {
> > > + state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate);
> > > + state->duty_cycle = state->period / 2;
> > > + state->polarity = PWM_POLARITY_NORMAL;
> > > + state->enabled = true;
> > > + return;
> > > + }
> > > +
> > >
> > > if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) &&
> > >
> > > sun4i_pwm->data->has_prescaler_bypass)
> > >
> > > prescaler = 1;
> > >
> > > @@ -211,7 +224,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip,
> > > struct pwm_device *pwm,>
> > > {
> > >
> > > struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
> > > struct pwm_state cstate;
> > >
> > > - u32 ctrl;
> > > + u32 ctrl, clk_rate;
> > > + bool bypass;
> > >
> > > int ret;
> > > unsigned int delay_us;
> > > unsigned long now;
> > >
> > > @@ -226,6 +240,16 @@ static int sun4i_pwm_apply(struct pwm_chip *chip,
> > > struct pwm_device *pwm,>
> > > }
> > >
> > > }
> > >
> > > + /*
> > > + * Although it would make much more sense to check for bypass in
> > > + * sun4i_pwm_calculate(), value of bypass bit also depends on "enabled".
> > > + * Period is allowed to be rounded up or down.
> > > + */
> >
> > Every driver seems to implement rounding the way its driver considers it
> > sensible. @Thierry: This is another patch where it would be good to have
> > a global directive about how rounding is supposed to work to provide the
> > users an reliable and uniform way to work with PWMs.
> >
> > > + clk_rate = clk_get_rate(sun4i_pwm->clk);
> > > + bypass = (state->period == NSEC_PER_SEC / clk_rate ||
> > > + state->period == DIV_ROUND_UP(NSEC_PER_SEC, clk_rate)) &&
> > > + state->enabled;
> >
> > Not sure if the compiler is clever enough to notice the obvious
> > optimisation with this code construct, but you can write this test in a
> > more clever way which has zero instead of up to two divisions. Something
> > like:
> >
> > bypass = ((state->period * clk_rate >= NSEC_PER_SEC &&
> >state->period * clk_rate < NSEC_PER_SEC + clk_rate) &&
> > state->enabled);
> >
> > In the commit log you write the motivation for using bypass is that it
> > allows to implement higher frequency then with the "normal" operation.
> > As you don't skip calculating the normal parameters requesting such a
> > high-frequency setting either errors out or doesn't catch the impossible
> > request. In both cases there is something to fix.
>
> It's the latter, otherwise it wouldn't work for my case. I'll fix the check
> and
> skip additional logic.
Great.
> > > +
> > >
> > > spin_lock(&sun4i_pwm->ctrl_lock);
> > > ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> > >
> > > @@ -273,6 +297,11 @@ static int sun4i_pwm_apply(struct pwm_chip *chip,
> > > struct pwm_device *pwm,>
> > > ctrl &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> > >
> > > }
> > >
> > > + if (bypass)
> > > + ctrl |= BIT_CH(PWM_BYPASS, pwm->hwpwm);
> > > + else
> > > + ctrl &= ~BIT_CH(PWM_BYPASS, pwm->hwpwm);
> > > +
> >
> > Does switching on (or off) the bypass bit complete the currently running
> > period?
>
> I don't really know. If I understand correctly, it just bypasses PWM logic
> completely, so I would say it doesn't complete the currently running period.
This is a bug. It's part of the promise of the PWM API that started
periods are completed. Please at least document this limitation at the
top of the
Re: [linux-sunxi] Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM
Hello,
On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> wrote:
> > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König
> > > napisal(a):
> > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > > > > sun4i_pwm_single_bypass = {>
> > > > > .npwm = 1,
> > > > >
> > > > > };
> > > > >
> > > > > +static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst = {
> > > > > + .has_bus_clock = true,
> > > > > + .has_prescaler_bypass = true,
> > > > > + .has_reset = true,
> > > > > + .npwm = 2,
> > > > > +};
> > > > > +
> > > > >
> > > > > static const struct of_device_id sun4i_pwm_dt_ids[] = {
> > > > >
> > > > > {
> > > > >
> > > > > .compatible = "allwinner,sun4i-a10-pwm",
> > > > >
> > > > > @@ -347,6 +354,9 @@ static const struct of_device_id
> > > > > sun4i_pwm_dt_ids[] =
> > > > > {
> > > > >
> > > > > }, {
> > > > >
> > > > > .compatible = "allwinner,sun8i-h3-pwm",
> > > > > .data = &sun4i_pwm_single_bypass,
> > > > >
> > > > > + }, {
> > > > > + .compatible = "allwinner,sun50i-h6-pwm",
> > > > > + .data = &sun50i_pwm_dual_bypass_clk_rst,
> > > >
> > > > If you follow my suggestion for the two previous patches, you can just
> > > > use:
> > > >
> > > > compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm";
> > > >
> > > > and drop this patch.
> > >
> > > Maxime found out that it's not compatible with A10s due to difference in
> > > bypass
> > > bit, but yes, I know what you mean.
> > >
> > > Since H6 requires reset line and bus clock to be specified, it's not
> > > compatible
> > > from DT binding side. New yaml based binding must somehow know that in
> > > order
> > > to be able to validate DT node, so it needs standalone compatible.
> > > However,
> > > depending on conclusions of other discussions, this new compatible can be
> > > associated with already available quirks structure or have it's own.
> >
> > I cannot follow. You should be able to specify in the binding that the
> > reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm
> > without a reset line and bus clock also verifies, but this doesn't
> > really hurt (and who knows, maybe the next allwinner chip needs exactly
> > this).
>
> It is not optional. It will not work if either the clocks or reset controls
> are missing. How would these be optional anyway? Either it's connected and
> thus required, or it's not and therefore should be omitted from the
> description.
[Just arguing about the clock here, the argumentation is analogous for
the reset control.]
>From the driver's perspective it's optional: There are devices with and
without a bus clock. This doesn't mean that you can just ignore this
clock if it's specified. It's optional in the sense "If dt doesn't
specify it, then assume this is a device that doesn't have it and so you
don't need to handle it." but not in the sense "it doesn't matter if
you handle it or not.".
Other than that I'm on your side. So for example I think it's not
optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that
devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n
because this hides exactly the kind of problem you point out here.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
To view this discussion on the web, visit
https://groups.google.com/d/msgid/linux-sunxi/20190729162428.bxuzgxg5sjqptlbp%40pengutronix.de.
[linux-sunxi] Re: [PATCH 5/6] pwm: sun4i: Add support to output source clock directly
Hi Uwe,
Dne ponedeljek, 29. julij 2019 ob 09:06:05 CEST je Uwe Kleine-König
napisal(a):
> On Fri, Jul 26, 2019 at 08:40:44PM +0200, Jernej Skrabec wrote:
> > PWM core has an option to bypass whole logic and output unchanged source
> > clock as PWM output. This is achieved by enabling bypass bit.
> >
> > Note that when bypass is enabled, no other setting has any meaning, not
> > even enable bit.
> >
> > This mode of operation is needed to achieve high enough frequency to
> > serve as clock source for AC200 chip, which is integrated into same
> > package as H6 SoC.
> >
> > Signed-off-by: Jernej Skrabec
> > ---
> >
> > drivers/pwm/pwm-sun4i.c | 31 ++-
> > 1 file changed, 30 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > index 9e0eca79ff88..848cff26f385 100644
> > --- a/drivers/pwm/pwm-sun4i.c
> > +++ b/drivers/pwm/pwm-sun4i.c
> > @@ -120,6 +120,19 @@ static void sun4i_pwm_get_state(struct pwm_chip
> > *chip,
> >
> > val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> >
> > + /*
> > +* PWM chapter in H6 manual has a diagram which explains that if
bypass
> > +* bit is set, no other setting has any meaning. Even more,
experiment
> > +* proved that also enable bit is ignored in this case.
> > +*/
> > + if (val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) {
> > + state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC,
clk_rate);
> > + state->duty_cycle = state->period / 2;
> > + state->polarity = PWM_POLARITY_NORMAL;
> > + state->enabled = true;
> > + return;
> > + }
> > +
> >
> > if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) &&
> >
> > sun4i_pwm->data->has_prescaler_bypass)
> >
> > prescaler = 1;
> >
> > @@ -211,7 +224,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip,
> > struct pwm_device *pwm,>
> > {
> >
> > struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
> > struct pwm_state cstate;
> >
> > - u32 ctrl;
> > + u32 ctrl, clk_rate;
> > + bool bypass;
> >
> > int ret;
> > unsigned int delay_us;
> > unsigned long now;
> >
> > @@ -226,6 +240,16 @@ static int sun4i_pwm_apply(struct pwm_chip *chip,
> > struct pwm_device *pwm,>
> > }
> >
> > }
> >
> > + /*
> > +* Although it would make much more sense to check for bypass in
> > +* sun4i_pwm_calculate(), value of bypass bit also depends on
"enabled".
> > +* Period is allowed to be rounded up or down.
> > +*/
>
> Every driver seems to implement rounding the way its driver considers it
> sensible. @Thierry: This is another patch where it would be good to have
> a global directive about how rounding is supposed to work to provide the
> users an reliable and uniform way to work with PWMs.
>
> > + clk_rate = clk_get_rate(sun4i_pwm->clk);
> > + bypass = (state->period == NSEC_PER_SEC / clk_rate ||
> > +state->period == DIV_ROUND_UP(NSEC_PER_SEC, clk_rate))
&&
> > +state->enabled;
>
> Not sure if the compiler is clever enough to notice the obvious
> optimisation with this code construct, but you can write this test in a
> more clever way which has zero instead of up to two divisions. Something
> like:
>
> bypass = ((state->period * clk_rate >= NSEC_PER_SEC &&
> state->period * clk_rate < NSEC_PER_SEC + clk_rate) &&
> state->enabled);
>
> In the commit log you write the motivation for using bypass is that it
> allows to implement higher frequency then with the "normal" operation.
> As you don't skip calculating the normal parameters requesting such a
> high-frequency setting either errors out or doesn't catch the impossible
> request. In both cases there is something to fix.
It's the latter, otherwise it wouldn't work for my case. I'll fix the check and
skip additional logic.
>
> > +
> >
> > spin_lock(&sun4i_pwm->ctrl_lock);
> > ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> >
> > @@ -273,6 +297,11 @@ static int sun4i_pwm_apply(struct pwm_chip *chip,
> > struct pwm_device *pwm,>
> > ctrl &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> >
> > }
> >
> > + if (bypass)
> > + ctrl |= BIT_CH(PWM_BYPASS, pwm->hwpwm);
> > + else
> > + ctrl &= ~BIT_CH(PWM_BYPASS, pwm->hwpwm);
> > +
>
> Does switching on (or off) the bypass bit complete the currently running
> period?
I don't really know. If I understand correctly, it just bypasses PWM logic
completely, so I would say it doesn't complete the currently running period.
Take a look at chapter 3.9.2 http://linux-sunxi.org/
File:Allwinner_H6_V200_User_Manual_V1.1.pdf
Best regards,
Jernej
>
> > sun4i_pwm_writel(sun4i_pwm, ctrl, PWM_CTRL_REG);
> >
> > spin_unlock(&sun4i_pwm->ctrl_lock);
>
> Best regards
> Uwe
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To u
[linux-sunxi] Re: [PATCH 3/6] pwm: sun4i: Add a quirk for bus clock
Hello Jernej,
On Mon, Jul 29, 2019 at 05:48:36PM +0200, Jernej Škrabec wrote:
> Dne ponedeljek, 29. julij 2019 ob 08:38:25 CEST je Uwe Kleine-König
> napisal(a):
> > Hello,
> >
> > On Fri, Jul 26, 2019 at 08:40:42PM +0200, Jernej Skrabec wrote:
> > > H6 PWM core needs bus clock to be enabled in order to work.
> > >
> > > Add a quirk for it.
> > >
> > > Signed-off-by: Jernej Skrabec
> > > ---
> > >
> > > drivers/pwm/pwm-sun4i.c | 15 +++
> > > 1 file changed, 15 insertions(+)
> > >
> > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > index 1b7be8fbde86..7d3ac3f2dc3f 100644
> > > --- a/drivers/pwm/pwm-sun4i.c
> > > +++ b/drivers/pwm/pwm-sun4i.c
> > > @@ -72,6 +72,7 @@ static const u32 prescaler_table[] = {
> > >
> > > };
> > >
> > > struct sun4i_pwm_data {
> > >
> > > + bool has_bus_clock;
> > >
> > > bool has_prescaler_bypass;
> > > bool has_reset;
> > > unsigned int npwm;
> > >
> > > @@ -79,6 +80,7 @@ struct sun4i_pwm_data {
> > >
> > > struct sun4i_pwm_chip {
> > >
> > > struct pwm_chip chip;
> > >
> > > + struct clk *bus_clk;
> > >
> > > struct clk *clk;
> > > struct reset_control *rst;
> > > void __iomem *base;
> > >
> > > @@ -382,6 +384,16 @@ static int sun4i_pwm_probe(struct platform_device
> > > *pdev)>
> > > reset_control_deassert(pwm->rst);
> > >
> > > }
> > >
> > > + if (pwm->data->has_bus_clock) {
> > > + pwm->bus_clk = devm_clk_get(&pdev->dev, "bus");
> >
> > Similar to my suggestion in patch 2: I'd use devm_clk_get_optional() and
> > drop struct sun4i_pwm_data::has_bus_clock.
>
> This one is not so simple. This patch has incorrect logic. Correct logic
> would
> be to use "devm_clk_get(&pdev->dev, NULL)" for variants without bus clock as
> it is done already and "devm_clk_get(&pdev->dev, "bus")" and
> "devm_clk_get(&pdev->dev, "mod")" for variants with bus clock.
Then maybe something like the following?:
busclk = devm_clk_get_optional(..., "bus");
modclk = devm_clk_get_optional(..., "mod");
/*
* old dtbs might have a single clock but no clock names. Fall
* back to this for compatibility reasons.
*/
if (!modclk) {
modclk = devm_clk_get(..., NULL);
}
> You see, DT nodes for other variants don't have clock-names property at all.
> If it would be specified, it would be "mod". So, DT nodes for other variants
> have "mod" clock specified on first place (the only one), while H6 DT node
> will
> have "mod" clock specified on second place (see one of e-mails from Maxime),
> so
> "NULL" can't be used instead of "mod" in both cases.
>
> So I would say quirk is beneficial to know if you have to look up clocks by
> name or you just take first clock specified.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
To view this discussion on the web, visit
https://groups.google.com/d/msgid/linux-sunxi/20190729161435.5bnj3ikocsyep4dd%40pengutronix.de.
Re: [linux-sunxi] Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM
On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
wrote:
>
> On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > Hi Uwe,
> >
> > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König
> > napisal(a):
> > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > Now that sun4i PWM driver supports deasserting reset line and enabling
> > > > bus clock, support for H6 PWM can be added.
> > > >
> > > > Note that while H6 PWM has two channels, only first one is wired to
> > > > output pin. Second channel is used as a clock source to companion AC200
> > > > chip which is bundled into same package.
> > > >
> > > > Signed-off-by: Jernej Skrabec
> > > > ---
> > > >
> > > > drivers/pwm/pwm-sun4i.c | 10 ++
> > > > 1 file changed, 10 insertions(+)
> > > >
> > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > > index 7d3ac3f2dc3f..9e0eca79ff88 100644
> > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > > > sun4i_pwm_single_bypass = {>
> > > > .npwm = 1,
> > > >
> > > > };
> > > >
> > > > +static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst = {
> > > > + .has_bus_clock = true,
> > > > + .has_prescaler_bypass = true,
> > > > + .has_reset = true,
> > > > + .npwm = 2,
> > > > +};
> > > > +
> > > >
> > > > static const struct of_device_id sun4i_pwm_dt_ids[] = {
> > > >
> > > > {
> > > >
> > > > .compatible = "allwinner,sun4i-a10-pwm",
> > > >
> > > > @@ -347,6 +354,9 @@ static const struct of_device_id sun4i_pwm_dt_ids[]
> > > > =
> > > > {
> > > >
> > > > }, {
> > > >
> > > > .compatible = "allwinner,sun8i-h3-pwm",
> > > > .data = &sun4i_pwm_single_bypass,
> > > >
> > > > + }, {
> > > > + .compatible = "allwinner,sun50i-h6-pwm",
> > > > + .data = &sun50i_pwm_dual_bypass_clk_rst,
> > >
> > > If you follow my suggestion for the two previous patches, you can just
> > > use:
> > >
> > > compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm";
> > >
> > > and drop this patch.
> >
> > Maxime found out that it's not compatible with A10s due to difference in
> > bypass
> > bit, but yes, I know what you mean.
> >
> > Since H6 requires reset line and bus clock to be specified, it's not
> > compatible
> > from DT binding side. New yaml based binding must somehow know that in order
> > to be able to validate DT node, so it needs standalone compatible. However,
> > depending on conclusions of other discussions, this new compatible can be
> > associated with already available quirks structure or have it's own.
>
> I cannot follow. You should be able to specify in the binding that the
> reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm
> without a reset line and bus clock also verifies, but this doesn't
> really hurt (and who knows, maybe the next allwinner chip needs exactly
> this).
It is not optional. It will not work if either the clocks or reset controls
are missing. How would these be optional anyway? Either it's connected and
thus required, or it's not and therefore should be omitted from the
description.
ChenYu
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
To view this discussion on the web, visit
https://groups.google.com/d/msgid/linux-sunxi/CAGb2v66C%3Dghjck6rxTg6Vt4xN2DcXntzVOa%3DKJWh98KRjkhnHQ%40mail.gmail.com.
[linux-sunxi] Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM
On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> Hi Uwe,
>
> Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König
> napisal(a):
> > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > Now that sun4i PWM driver supports deasserting reset line and enabling
> > > bus clock, support for H6 PWM can be added.
> > >
> > > Note that while H6 PWM has two channels, only first one is wired to
> > > output pin. Second channel is used as a clock source to companion AC200
> > > chip which is bundled into same package.
> > >
> > > Signed-off-by: Jernej Skrabec
> > > ---
> > >
> > > drivers/pwm/pwm-sun4i.c | 10 ++
> > > 1 file changed, 10 insertions(+)
> > >
> > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > index 7d3ac3f2dc3f..9e0eca79ff88 100644
> > > --- a/drivers/pwm/pwm-sun4i.c
> > > +++ b/drivers/pwm/pwm-sun4i.c
> > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > > sun4i_pwm_single_bypass = {>
> > > .npwm = 1,
> > >
> > > };
> > >
> > > +static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst = {
> > > + .has_bus_clock = true,
> > > + .has_prescaler_bypass = true,
> > > + .has_reset = true,
> > > + .npwm = 2,
> > > +};
> > > +
> > >
> > > static const struct of_device_id sun4i_pwm_dt_ids[] = {
> > >
> > > {
> > >
> > > .compatible = "allwinner,sun4i-a10-pwm",
> > >
> > > @@ -347,6 +354,9 @@ static const struct of_device_id sun4i_pwm_dt_ids[] =
> > > {
> > >
> > > }, {
> > >
> > > .compatible = "allwinner,sun8i-h3-pwm",
> > > .data = &sun4i_pwm_single_bypass,
> > >
> > > + }, {
> > > + .compatible = "allwinner,sun50i-h6-pwm",
> > > + .data = &sun50i_pwm_dual_bypass_clk_rst,
> >
> > If you follow my suggestion for the two previous patches, you can just
> > use:
> >
> > compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm";
> >
> > and drop this patch.
>
> Maxime found out that it's not compatible with A10s due to difference in
> bypass
> bit, but yes, I know what you mean.
>
> Since H6 requires reset line and bus clock to be specified, it's not
> compatible
> from DT binding side. New yaml based binding must somehow know that in order
> to be able to validate DT node, so it needs standalone compatible. However,
> depending on conclusions of other discussions, this new compatible can be
> associated with already available quirks structure or have it's own.
I cannot follow. You should be able to specify in the binding that the
reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm
without a reset line and bus clock also verifies, but this doesn't
really hurt (and who knows, maybe the next allwinner chip needs exactly
this).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
To view this discussion on the web, visit
https://groups.google.com/d/msgid/linux-sunxi/20190729160723.am3cs5pasi22pibi%40pengutronix.de.
[linux-sunxi] Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM
Hi Uwe,
Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König
napisal(a):
> On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > Now that sun4i PWM driver supports deasserting reset line and enabling
> > bus clock, support for H6 PWM can be added.
> >
> > Note that while H6 PWM has two channels, only first one is wired to
> > output pin. Second channel is used as a clock source to companion AC200
> > chip which is bundled into same package.
> >
> > Signed-off-by: Jernej Skrabec
> > ---
> >
> > drivers/pwm/pwm-sun4i.c | 10 ++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > index 7d3ac3f2dc3f..9e0eca79ff88 100644
> > --- a/drivers/pwm/pwm-sun4i.c
> > +++ b/drivers/pwm/pwm-sun4i.c
> > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > sun4i_pwm_single_bypass = {>
> > .npwm = 1,
> >
> > };
> >
> > +static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst = {
> > + .has_bus_clock = true,
> > + .has_prescaler_bypass = true,
> > + .has_reset = true,
> > + .npwm = 2,
> > +};
> > +
> >
> > static const struct of_device_id sun4i_pwm_dt_ids[] = {
> >
> > {
> >
> > .compatible = "allwinner,sun4i-a10-pwm",
> >
> > @@ -347,6 +354,9 @@ static const struct of_device_id sun4i_pwm_dt_ids[] =
> > {
> >
> > }, {
> >
> > .compatible = "allwinner,sun8i-h3-pwm",
> > .data = &sun4i_pwm_single_bypass,
> >
> > + }, {
> > + .compatible = "allwinner,sun50i-h6-pwm",
> > + .data = &sun50i_pwm_dual_bypass_clk_rst,
>
> If you follow my suggestion for the two previous patches, you can just
> use:
>
> compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm";
>
> and drop this patch.
Maxime found out that it's not compatible with A10s due to difference in bypass
bit, but yes, I know what you mean.
Since H6 requires reset line and bus clock to be specified, it's not compatible
from DT binding side. New yaml based binding must somehow know that in order
to be able to validate DT node, so it needs standalone compatible. However,
depending on conclusions of other discussions, this new compatible can be
associated with already available quirks structure or have it's own.
Best regards,
Jernej
>
> Best regards
> Uwe
>
> > }, {
> >
> > /* sentinel */
> >
> > },
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
To view this discussion on the web, visit
https://groups.google.com/d/msgid/linux-sunxi/223488703.0I5IR7NXoI%40jernej-laptop.
[linux-sunxi] Re: [PATCH 3/6] pwm: sun4i: Add a quirk for bus clock
Hi Uwe,
Dne ponedeljek, 29. julij 2019 ob 08:38:25 CEST je Uwe Kleine-König
napisal(a):
> Hello,
>
> On Fri, Jul 26, 2019 at 08:40:42PM +0200, Jernej Skrabec wrote:
> > H6 PWM core needs bus clock to be enabled in order to work.
> >
> > Add a quirk for it.
> >
> > Signed-off-by: Jernej Skrabec
> > ---
> >
> > drivers/pwm/pwm-sun4i.c | 15 +++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > index 1b7be8fbde86..7d3ac3f2dc3f 100644
> > --- a/drivers/pwm/pwm-sun4i.c
> > +++ b/drivers/pwm/pwm-sun4i.c
> > @@ -72,6 +72,7 @@ static const u32 prescaler_table[] = {
> >
> > };
> >
> > struct sun4i_pwm_data {
> >
> > + bool has_bus_clock;
> >
> > bool has_prescaler_bypass;
> > bool has_reset;
> > unsigned int npwm;
> >
> > @@ -79,6 +80,7 @@ struct sun4i_pwm_data {
> >
> > struct sun4i_pwm_chip {
> >
> > struct pwm_chip chip;
> >
> > + struct clk *bus_clk;
> >
> > struct clk *clk;
> > struct reset_control *rst;
> > void __iomem *base;
> >
> > @@ -382,6 +384,16 @@ static int sun4i_pwm_probe(struct platform_device
> > *pdev)>
> > reset_control_deassert(pwm->rst);
> >
> > }
> >
> > + if (pwm->data->has_bus_clock) {
> > + pwm->bus_clk = devm_clk_get(&pdev->dev, "bus");
>
> Similar to my suggestion in patch 2: I'd use devm_clk_get_optional() and
> drop struct sun4i_pwm_data::has_bus_clock.
This one is not so simple. This patch has incorrect logic. Correct logic would
be to use "devm_clk_get(&pdev->dev, NULL)" for variants without bus clock as
it is done already and "devm_clk_get(&pdev->dev, "bus")" and
"devm_clk_get(&pdev->dev, "mod")" for variants with bus clock.
You see, DT nodes for other variants don't have clock-names property at all.
If it would be specified, it would be "mod". So, DT nodes for other variants
have "mod" clock specified on first place (the only one), while H6 DT node will
have "mod" clock specified on second place (see one of e-mails from Maxime), so
"NULL" can't be used instead of "mod" in both cases.
So I would say quirk is beneficial to know if you have to look up clocks by
name or you just take first clock specified.
Best regards,
Jernej
>
> Best regards
> Uwe
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
To view this discussion on the web, visit
https://groups.google.com/d/msgid/linux-sunxi/4022372.WfP88Fa4Lu%40jernej-laptop.
[linux-sunxi] [PATCH] ASoC: sun4i-i2s: Incorrect SR and WSS computation
From: Marcus Cooper
The A64 audio codec uses the original I2S block but the SR and
WSS computation currently assigned is for the newer block.
Fixes: 619c15f7fac9 (ASoC: sun4i-i2s: Change SR and WSS computation)
Signed-off-by: Marcus Cooper
---
sound/soc/sunxi/sun4i-i2s.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index 9b2232908b65..7fa5c61169db 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -1002,8 +1002,8 @@ static const struct sun4i_i2s_quirks
sun50i_a64_codec_i2s_quirks = {
.field_rxchanmap= REG_FIELD(SUN4I_I2S_RX_CHAN_MAP_REG, 0, 31),
.field_txchansel= REG_FIELD(SUN4I_I2S_TX_CHAN_SEL_REG, 0, 2),
.field_rxchansel= REG_FIELD(SUN4I_I2S_RX_CHAN_SEL_REG, 0, 2),
- .get_sr = sun8i_i2s_get_sr_wss,
- .get_wss= sun8i_i2s_get_sr_wss,
+ .get_sr = sun4i_i2s_get_sr,
+ .get_wss= sun4i_i2s_get_wss,
};
static int sun4i_i2s_init_regmap_fields(struct device *dev,
--
2.22.0
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
To view this discussion on the web, visit
https://groups.google.com/d/msgid/linux-sunxi/20190729152130.27955-1-codekipper%40gmail.com.
[linux-sunxi] Re: [PATCH 2/6] pwm: sun4i: Add a quirk for reset line
Hi,
On Mon, 2019-07-29 at 09:12 +0200, Uwe Kleine-König wrote:
> Hello,
>
> On Mon, Jul 29, 2019 at 02:43:23PM +0800, Chen-Yu Tsai wrote:
> > On Mon, Jul 29, 2019 at 2:36 PM Uwe Kleine-König
> > wrote:
> > > On Fri, Jul 26, 2019 at 08:40:41PM +0200, Jernej Skrabec wrote:
> > > > @@ -371,6 +374,14 @@ static int sun4i_pwm_probe(struct platform_device
> > > > *pdev)
> > > > if (IS_ERR(pwm->clk))
> > > > return PTR_ERR(pwm->clk);
> > > >
> > > > + if (pwm->data->has_reset) {
> > > > + pwm->rst = devm_reset_control_get(&pdev->dev, NULL);
> > > > + if (IS_ERR(pwm->rst))
> > > > + return PTR_ERR(pwm->rst);
> > > > +
> > > > + reset_control_deassert(pwm->rst);
> > > > + }
> > > > +
> > >
> > > I wonder why there is a need to track if a given chip needs a reset
> > > line. I'd just use devm_reset_control_get_optional() and drop the
> > > .has_reset member in struct sun4i_pwm_data.
> >
> > Because it's not optional for this platform, i.e. it won't work if
> > the reset control (or clk, in the next patch) is somehow missing from
> > the device tree.
>
> If the device tree is wrong it is considered ok that the driver doesn't
> behave correctly. So this is not a problem you need (or should) care
> about.
I agree with this. Catching missing DT properties and other device tree
validation is not the job of device drivers. The _optional request
variants were introduced to simplify drivers that require the reset line
on some platforms and not on others.
I would ask to explicitly state whether the driver needs full control
over the moment of (de)assertion of the reset signal, or whether the
only requirement is that the reset signal stays deasserted while the PWM
driver is active, by using devm_reset_control_get_optional_exclusive or
devm_reset_control_get_optional_shared to request the reset control.
regards
Philipp
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
To view this discussion on the web, visit
https://groups.google.com/d/msgid/linux-sunxi/1564395538.7633.7.camel%40pengutronix.de.
[linux-sunxi] Re: [PATCH 2/6] pwm: sun4i: Add a quirk for reset line
Hello,
On Mon, Jul 29, 2019 at 02:43:23PM +0800, Chen-Yu Tsai wrote:
> On Mon, Jul 29, 2019 at 2:36 PM Uwe Kleine-König
> wrote:
> > On Fri, Jul 26, 2019 at 08:40:41PM +0200, Jernej Skrabec wrote:
> > > @@ -371,6 +374,14 @@ static int sun4i_pwm_probe(struct platform_device
> > > *pdev)
> > > if (IS_ERR(pwm->clk))
> > > return PTR_ERR(pwm->clk);
> > >
> > > + if (pwm->data->has_reset) {
> > > + pwm->rst = devm_reset_control_get(&pdev->dev, NULL);
> > > + if (IS_ERR(pwm->rst))
> > > + return PTR_ERR(pwm->rst);
> > > +
> > > + reset_control_deassert(pwm->rst);
> > > + }
> > > +
> >
> > I wonder why there is a need to track if a given chip needs a reset
> > line. I'd just use devm_reset_control_get_optional() and drop the
> > .has_reset member in struct sun4i_pwm_data.
>
> Because it's not optional for this platform, i.e. it won't work if
> the reset control (or clk, in the next patch) is somehow missing from
> the device tree.
If the device tree is wrong it is considered ok that the driver doesn't
behave correctly. So this is not a problem you need (or should) care
about.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
To view this discussion on the web, visit
https://groups.google.com/d/msgid/linux-sunxi/20190729071218.bukw7vxilqy523k3%40pengutronix.de.
[linux-sunxi] Re: [PATCH 5/6] pwm: sun4i: Add support to output source clock directly
On Fri, Jul 26, 2019 at 08:40:44PM +0200, Jernej Skrabec wrote:
> PWM core has an option to bypass whole logic and output unchanged source
> clock as PWM output. This is achieved by enabling bypass bit.
>
> Note that when bypass is enabled, no other setting has any meaning, not
> even enable bit.
>
> This mode of operation is needed to achieve high enough frequency to
> serve as clock source for AC200 chip, which is integrated into same
> package as H6 SoC.
>
> Signed-off-by: Jernej Skrabec
> ---
> drivers/pwm/pwm-sun4i.c | 31 ++-
> 1 file changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index 9e0eca79ff88..848cff26f385 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -120,6 +120,19 @@ static void sun4i_pwm_get_state(struct pwm_chip *chip,
>
> val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
>
> + /*
> + * PWM chapter in H6 manual has a diagram which explains that if bypass
> + * bit is set, no other setting has any meaning. Even more, experiment
> + * proved that also enable bit is ignored in this case.
> + */
> + if (val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) {
> + state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate);
> + state->duty_cycle = state->period / 2;
> + state->polarity = PWM_POLARITY_NORMAL;
> + state->enabled = true;
> + return;
> + }
> +
> if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) &&
> sun4i_pwm->data->has_prescaler_bypass)
> prescaler = 1;
> @@ -211,7 +224,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct
> pwm_device *pwm,
> {
> struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
> struct pwm_state cstate;
> - u32 ctrl;
> + u32 ctrl, clk_rate;
> + bool bypass;
> int ret;
> unsigned int delay_us;
> unsigned long now;
> @@ -226,6 +240,16 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct
> pwm_device *pwm,
> }
> }
>
> + /*
> + * Although it would make much more sense to check for bypass in
> + * sun4i_pwm_calculate(), value of bypass bit also depends on "enabled".
> + * Period is allowed to be rounded up or down.
> + */
Every driver seems to implement rounding the way its driver considers it
sensible. @Thierry: This is another patch where it would be good to have
a global directive about how rounding is supposed to work to provide the
users an reliable and uniform way to work with PWMs.
> + clk_rate = clk_get_rate(sun4i_pwm->clk);
> + bypass = (state->period == NSEC_PER_SEC / clk_rate ||
> + state->period == DIV_ROUND_UP(NSEC_PER_SEC, clk_rate)) &&
> + state->enabled;
Not sure if the compiler is clever enough to notice the obvious
optimisation with this code construct, but you can write this test in a
more clever way which has zero instead of up to two divisions. Something
like:
bypass = ((state->period * clk_rate >= NSEC_PER_SEC &&
state->period * clk_rate < NSEC_PER_SEC + clk_rate) &&
state->enabled);
In the commit log you write the motivation for using bypass is that it
allows to implement higher frequency then with the "normal" operation.
As you don't skip calculating the normal parameters requesting such a
high-frequency setting either errors out or doesn't catch the impossible
request. In both cases there is something to fix.
> +
> spin_lock(&sun4i_pwm->ctrl_lock);
> ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
>
> @@ -273,6 +297,11 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct
> pwm_device *pwm,
> ctrl &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> }
>
> + if (bypass)
> + ctrl |= BIT_CH(PWM_BYPASS, pwm->hwpwm);
> + else
> + ctrl &= ~BIT_CH(PWM_BYPASS, pwm->hwpwm);
> +
Does switching on (or off) the bypass bit complete the currently running
period?
> sun4i_pwm_writel(sun4i_pwm, ctrl, PWM_CTRL_REG);
>
> spin_unlock(&sun4i_pwm->ctrl_lock);
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
To view this discussion on the web, visit
https://groups.google.com/d/msgid/linux-sunxi/20190729070605.vlu7kgzn362ph2q3%40pengutronix.de.
