[PATCH 1/3] ASoC:codec:max98373: Changed amp shutdown register as volatile
0x20FF(amp global enable) register was defined as non-volatile, but it is not. Overheating, overcurrent can cause amp shutdown in hardware. 'regmap_write' compare register readback value before writing to avoid same value writing. 'regmap_read' just read cache not actual hardware value for the non-volatile register. When amp is internally shutdown by some reason, next 'AMP ON' command can be ignored because regmap think amp is already ON. Signed-off-by: Ryan Lee --- sound/soc/codecs/max98373-i2c.c | 1 + sound/soc/codecs/max98373-sdw.c | 1 + 2 files changed, 2 insertions(+) diff --git a/sound/soc/codecs/max98373-i2c.c b/sound/soc/codecs/max98373-i2c.c index 85f6865019d4..ddb6436835d7 100644 --- a/sound/soc/codecs/max98373-i2c.c +++ b/sound/soc/codecs/max98373-i2c.c @@ -446,6 +446,7 @@ static bool max98373_volatile_reg(struct device *dev, unsigned int reg) case MAX98373_R2054_MEAS_ADC_PVDD_CH_READBACK: case MAX98373_R2055_MEAS_ADC_THERM_CH_READBACK: case MAX98373_R20B6_BDE_CUR_STATE_READBACK: + case MAX98373_R20FF_GLOBAL_SHDN: case MAX98373_R21FF_REV_ID: return true; default: diff --git a/sound/soc/codecs/max98373-sdw.c b/sound/soc/codecs/max98373-sdw.c index d8c47667a9ea..f3a12205cd48 100644 --- a/sound/soc/codecs/max98373-sdw.c +++ b/sound/soc/codecs/max98373-sdw.c @@ -220,6 +220,7 @@ static bool max98373_volatile_reg(struct device *dev, unsigned int reg) case MAX98373_R2054_MEAS_ADC_PVDD_CH_READBACK: case MAX98373_R2055_MEAS_ADC_THERM_CH_READBACK: case MAX98373_R20B6_BDE_CUR_STATE_READBACK: + case MAX98373_R20FF_GLOBAL_SHDN: case MAX98373_R21FF_REV_ID: /* SoundWire Control Port Registers */ case MAX98373_R0040_SCP_INIT_STAT_1 ... MAX98373_R0070_SCP_FRAME_CTLR: -- 2.17.1
[PATCH 3/3] ASoC:codec:max98373: Added controls for autorestart config
3 new controls are added.
"OVC Autorestart Switch" : controls whether or not the speaker amplifier
automatically re-enables after an overcurrent fault condition.
"THERM Autorestart Switch" : controls whether or not the device
automatically resumes playback when the die temperature recovers from
thermal shutdown.
"CMON Autorestart Switch" : controls whether or not the device
automatically resumes playback when the clock returns after stopping.
Above Auto Restart functions are enabled by default.
Signed-off-by: Ryan Lee
---
sound/soc/codecs/max98373.c | 14 ++
sound/soc/codecs/max98373.h | 3 +++
2 files changed, 17 insertions(+)
diff --git a/sound/soc/codecs/max98373.c b/sound/soc/codecs/max98373.c
index 1346a98ce8a1..e14fe98349a5 100644
--- a/sound/soc/codecs/max98373.c
+++ b/sound/soc/codecs/max98373.c
@@ -204,6 +204,15 @@ SOC_SINGLE("Ramp Up Switch", MAX98373_R203F_AMP_DSP_CFG,
MAX98373_AMP_DSP_CFG_RMP_UP_SHIFT, 1, 0),
SOC_SINGLE("Ramp Down Switch", MAX98373_R203F_AMP_DSP_CFG,
MAX98373_AMP_DSP_CFG_RMP_DN_SHIFT, 1, 0),
+/* Speaker Amplifier Overcurrent Automatic Restart Enable */
+SOC_SINGLE("OVC Autorestart Switch", MAX98373_R20FE_DEVICE_AUTO_RESTART_CFG,
+ MAX98373_OVC_AUTORESTART_SHIFT, 1, 0),
+/* Thermal Shutdown Automatic Restart Enable */
+SOC_SINGLE("THERM Autorestart Switch", MAX98373_R20FE_DEVICE_AUTO_RESTART_CFG,
+ MAX98373_THERM_AUTORESTART_SHIFT, 1, 0),
+/* Clock Monitor Automatic Restart Enable */
+SOC_SINGLE("CMON Autorestart Switch", MAX98373_R20FE_DEVICE_AUTO_RESTART_CFG,
+ MAX98373_CMON_AUTORESTART_SHIFT, 1, 0),
SOC_SINGLE("CLK Monitor Switch", MAX98373_R20FE_DEVICE_AUTO_RESTART_CFG,
MAX98373_CLOCK_MON_SHIFT, 1, 0),
SOC_SINGLE("Dither Switch", MAX98373_R203F_AMP_DSP_CFG,
@@ -392,6 +401,11 @@ static int max98373_probe(struct snd_soc_component
*component)
MAX98373_R2021_PCM_TX_HIZ_EN_2,
1 << (max98373->i_slot - 8), 0);
+ /* enable auto restart function by default */
+ regmap_write(max98373->regmap,
+ MAX98373_R20FE_DEVICE_AUTO_RESTART_CFG,
+ 0xF);
+
/* speaker feedback slot configuration */
regmap_write(max98373->regmap,
MAX98373_R2023_PCM_TX_SRC_2,
diff --git a/sound/soc/codecs/max98373.h b/sound/soc/codecs/max98373.h
index 71f5a5228f34..73a2cf69d84a 100644
--- a/sound/soc/codecs/max98373.h
+++ b/sound/soc/codecs/max98373.h
@@ -195,6 +195,9 @@
#define MAX98373_LIMITER_EN_SHIFT (0)
/* MAX98373_R20FE_DEVICE_AUTO_RESTART_CFG */
+#define MAX98373_OVC_AUTORESTART_SHIFT (3)
+#define MAX98373_THERM_AUTORESTART_SHIFT (2)
+#define MAX98373_CMON_AUTORESTART_SHIFT (1)
#define MAX98373_CLOCK_MON_SHIFT (0)
/* MAX98373_R20FF_GLOBAL_SHDN */
--
2.17.1
[PATCH 2/3] ASoC:codec:max98373: Added 30ms turn on/off time delay
Amp requires 10 ~ 30ms for the power ON and OFF. Added 30ms delay for stability. Signed-off-by: Ryan Lee --- sound/soc/codecs/max98373.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sound/soc/codecs/max98373.c b/sound/soc/codecs/max98373.c index 746c829312b8..1346a98ce8a1 100644 --- a/sound/soc/codecs/max98373.c +++ b/sound/soc/codecs/max98373.c @@ -28,11 +28,13 @@ static int max98373_dac_event(struct snd_soc_dapm_widget *w, regmap_update_bits(max98373->regmap, MAX98373_R20FF_GLOBAL_SHDN, MAX98373_GLOBAL_EN_MASK, 1); + usleep_range(3, 31000); break; case SND_SOC_DAPM_POST_PMD: regmap_update_bits(max98373->regmap, MAX98373_R20FF_GLOBAL_SHDN, MAX98373_GLOBAL_EN_MASK, 0); + usleep_range(3, 31000); max98373->tdm_mode = false; break; default: -- 2.17.1
[PATCH 1/1] ipmi/watchdog: Add WDIOC_GETTIMELEFT ioctl
This is the same ioctl the rest of the watchdogs support. GETTIMELEFT
returns the number of seconds in the countdown -- useful for testing
whether the watchdog is functioning.
Signed-off-by: Ryan O'Leary
---
drivers/char/ipmi/ipmi_watchdog.c | 75 +++
1 file changed, 75 insertions(+)
diff --git a/drivers/char/ipmi/ipmi_watchdog.c
b/drivers/char/ipmi/ipmi_watchdog.c
index 32c334e34d55..f253c8667395 100644
--- a/drivers/char/ipmi/ipmi_watchdog.c
+++ b/drivers/char/ipmi/ipmi_watchdog.c
@@ -456,6 +456,71 @@ static int ipmi_set_timeout(int do_heartbeat)
return rv;
}
+static unsigned int __ipmi_get_timeout(struct ipmi_smi_msg *smi_msg,
+ struct ipmi_recv_msg *recv_msg,
+ int *countdown)
+{
+ struct kernel_ipmi_msgmsg;
+ int rv = 0;
+ struct ipmi_system_interface_addr addr;
+
+
+ addr.addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE;
+ addr.channel = IPMI_BMC_CHANNEL;
+ addr.lun = 0;
+
+ msg.netfn = 0x06;
+ msg.cmd = IPMI_WDOG_GET_TIMER;
+ msg.data = NULL;
+ msg.data_len = 0;
+ rv = ipmi_request_supply_msgs(watchdog_user,
+ (struct ipmi_addr *) ,
+ 0,
+ ,
+ NULL,
+ smi_msg,
+ recv_msg,
+ 1);
+ if (rv) {
+ pr_warn("get timeout error: %d\n", rv);
+ return rv;
+ }
+
+ wait_for_completion(_wait);
+
+ if (recv_msg->msg.data_len < 9) {
+ pr_warn("get timeout response size: %d (expected 9)\n",
+ recv_msg->msg.data_len);
+ return -EIO;
+ }
+
+ if (recv_msg->msg.data[0] != 0) {
+ pr_warn("get timeout completion code error: %d\n",
+ recv_msg->msg.data[0]);
+ return -EIO;
+ }
+
+ *countdown = WDOG_GET_TIMEOUT(recv_msg->msg.data[7],
recv_msg->msg.data[8]);
+
+ return rv;
+}
+
+static int _ipmi_get_timeout(int *countdown)
+{
+ int rv;
+
+ if (!watchdog_user)
+ return -ENODEV;
+
+ atomic_set(_tofree, 2);
+
+ rv = __ipmi_get_timeout(_msg,
+ _msg,
+ countdown);
+
+ return rv;
+}
+
static atomic_t panic_done_count = ATOMIC_INIT(0);
static void panic_smi_free(struct ipmi_smi_msg *msg)
@@ -729,6 +794,16 @@ static int ipmi_ioctl(struct file *file,
return -EFAULT;
return 0;
+ case WDIOC_GETTIMELEFT:
+ val = 0;
+ i = _ipmi_get_timeout();
+ if (i)
+ return i;
+ i = copy_to_user(argp, , sizeof(val));
+ if (i)
+ return -EFAULT;
+ return 0;
+
default:
return -ENOIOCTLCMD;
}
--
2.31.0.rc2.261.g7f71774620-goog
RE: [PATCH 1/1] clk: aspeed: modify some default clks are critical
> -Original Message- > From: Andrew Jeffery > Sent: Monday, January 25, 2021 8:47 AM > To: Ryan Chen ; Samuel Holland > ; Stephen Boyd ; Joel Stanley > > Cc: BMC-SW ; linux-aspeed > ; Michael Turquette > ; Linux Kernel Mailing List > ; linux-...@vger.kernel.org; Linux ARM > > Subject: Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical > > > > On Fri, 22 Jan 2021, at 18:45, Ryan Chen wrote: > > Hello, > > How about this patch progress? > > It does impact a lot of machine that when BMC boot at u-boot. > > SUART is work for Host. But after boot into kernel, due to the clk > > disabled. > > The SUART is not work for Host anymore. > > Maybe it's worth taking Ryan's patch for now, and when the protected-clocks > binding gets merged we can rip out the CLK_IS_CRITICAL flags and convert the > Aspeed devicetrees to use protected-clocks instead? > > The only issue I see with that plan is it becomes ambiguous as to which clock > each platform considers crititical/in-need-of-protection. > Hello Joel, Will you take this patch? Or you have another approach I may modify for it. Regards, Ryan > > > -Original Message- > > > From: Samuel Holland > > > Sent: Thursday, October 29, 2020 10:25 AM > > > To: Stephen Boyd ; Joel Stanley > > > Cc: Andrew Jeffery ; Michael Turquette > > > ; Ryan Chen ; > > > BMC-SW ; Linux ARM > > > ; linux-aspeed > > > ; linux-...@vger.kernel.org; Linux > > > Kernel Mailing List > > > Subject: Re: Re: [PATCH 1/1] clk: aspeed: modify some default clks > > > are critical > > > > > > Stephen, > > > > > > On 10/14/20 12:16 PM, Stephen Boyd wrote: > > > > Quoting Joel Stanley (2020-10-13 22:28:00) > > > >> On Wed, 14 Oct 2020 at 02:50, Stephen Boyd > wrote: > > > >>> > > > >>> Quoting Ryan Chen (2020-09-28 00:01:08) > > > >>>> In ASPEED SoC LCLK is LPC clock for all SuperIO device, > > > >>>> UART1/UART2 are default for Host SuperIO UART device, eSPI clk > > > >>>> for Host eSPI bus access eSPI slave channel, those clks can't > > > >>>> be disable should keep default, otherwise will affect Host side > > > >>>> access SuperIO and SPI slave > > > device. > > > >>>> > > > >>>> Signed-off-by: Ryan Chen > > > >>>> --- > > > >>> > > > >>> Is there resolution on this thread? > > > >> > > > >> Not yet. > > > >> > > > >> We have a system where the BMC (management controller) controls > > > >> some clocks, but the peripherals that it's clocking are outside > > > >> the BMC's control. In this case, the host processor us using some > > > >> UARTs and what not independent of any code running on the BMC. > > > >> > > > >> Ryan wants to have them marked as critical so the BMC never > > > >> powers them > > > down. > > > >> > > > >> However, there are systems that don't use this part of the soc, > > > >> so for those implementations they are not critical and Linux on > > > >> the BMC can turn them off. > > > >> > > > >> Do you have any thoughts? Has anyone solved a similar problem > already? > > > >> > > > > > > > > Is this critical clocks in DT? Where we want to have different DT > > > > for different device configurations to indicate that some clks > > > > should be marked critical so they're never turned off and other > > > > times they aren't so they're turned off? > > > > > > > > It also sounds sort of like the protected-clocks binding. Where > > > > you don't want to touch certain clks depending on the usage > > > > configuration of the SoC. There is a patch to make that generic > > > > that I haven't applied because it looks wrong at first glance[1]. > > > > Maybe not registering those clks to the framework on the > > > > configuration that Ryan has is > > > good enough? > > > > > > Could you please be more specific than the patch "looks wrong"? I'm > > > more than happy to update the patch to address your concerns, but I > > > cannot do that unless I know what your concerns are. > > > > > > Regards, > > > Samuel > > > > > > > [1] > > > > https://lore.kernel.org/r/20200903040015.5627-2-sam...@sholland.or > > > > g > > ___ > > linux-arm-kernel mailing list > > linux-arm-ker...@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > >
RE: Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical
Hello, How about this patch progress? It does impact a lot of machine that when BMC boot at u-boot. SUART is work for Host. But after boot into kernel, due to the clk disabled. The SUART is not work for Host anymore. Regards, Ryan > -Original Message- > From: Samuel Holland > Sent: Thursday, October 29, 2020 10:25 AM > To: Stephen Boyd ; Joel Stanley > Cc: Andrew Jeffery ; Michael Turquette > ; Ryan Chen ; > BMC-SW ; Linux ARM > ; linux-aspeed > ; linux-...@vger.kernel.org; Linux Kernel > Mailing List > Subject: Re: Re: [PATCH 1/1] clk: aspeed: modify some default clks are > critical > > Stephen, > > On 10/14/20 12:16 PM, Stephen Boyd wrote: > > Quoting Joel Stanley (2020-10-13 22:28:00) > >> On Wed, 14 Oct 2020 at 02:50, Stephen Boyd wrote: > >>> > >>> Quoting Ryan Chen (2020-09-28 00:01:08) > >>>> In ASPEED SoC LCLK is LPC clock for all SuperIO device, UART1/UART2 > >>>> are default for Host SuperIO UART device, eSPI clk for Host eSPI > >>>> bus access eSPI slave channel, those clks can't be disable should > >>>> keep default, otherwise will affect Host side access SuperIO and SPI > >>>> slave > device. > >>>> > >>>> Signed-off-by: Ryan Chen > >>>> --- > >>> > >>> Is there resolution on this thread? > >> > >> Not yet. > >> > >> We have a system where the BMC (management controller) controls some > >> clocks, but the peripherals that it's clocking are outside the BMC's > >> control. In this case, the host processor us using some UARTs and > >> what not independent of any code running on the BMC. > >> > >> Ryan wants to have them marked as critical so the BMC never powers them > down. > >> > >> However, there are systems that don't use this part of the soc, so > >> for those implementations they are not critical and Linux on the BMC > >> can turn them off. > >> > >> Do you have any thoughts? Has anyone solved a similar problem already? > >> > > > > Is this critical clocks in DT? Where we want to have different DT for > > different device configurations to indicate that some clks should be > > marked critical so they're never turned off and other times they > > aren't so they're turned off? > > > > It also sounds sort of like the protected-clocks binding. Where you > > don't want to touch certain clks depending on the usage configuration > > of the SoC. There is a patch to make that generic that I haven't > > applied because it looks wrong at first glance[1]. Maybe not > > registering those clks to the framework on the configuration that Ryan has > > is > good enough? > > Could you please be more specific than the patch "looks wrong"? I'm more > than happy to update the patch to address your concerns, but I cannot do that > unless I know what your concerns are. > > Regards, > Samuel > > > [1] > > https://lore.kernel.org/r/20200903040015.5627-2-sam...@sholland.org
RE: [PATCH v3 4/4] hwmon: Support Aspeed AST2600 PWM/Fan tachometer
> -Original Message- > From: Andrew Jeffery > Sent: Wednesday, January 20, 2021 1:16 PM > To: Troy Lee ; open...@lists.ozlabs.org; Joel > Stanley ; Philipp Zabel ; open list > ; moderated list:ARM/ASPEED MACHINE > SUPPORT ; moderated > list:ARM/ASPEED MACHINE SUPPORT > Cc: Ryan Chen ; ChiaWei Wang > ; Troy Lee ; kbuild > test robot > Subject: Re: [PATCH v3 4/4] hwmon: Support Aspeed AST2600 PWM/Fan > tachometer > > Hi Troy, > > On Mon, 18 Jan 2021, at 17:20, Troy Lee wrote: > > Add Aspeed AST2600 PWM/Fan tacho driver. AST2600 has 16 PWM channel > > and > > 16 FAN tacho channel. > > > > Changes since v2: > > - declare local function as static function > > > > Changes since v1: > > - fixed review comments > > - fixed double-looped calculation of div_h and div_l > > - moving configuration to device tree > > - register hwmon driver with devm_hwmon_device_register_with_info() > > > > Signed-off-by: Troy Lee > > Reported-by: kernel test robot > > --- > > drivers/hwmon/Kconfig| 10 + > > drivers/hwmon/Makefile | 1 + > > drivers/hwmon/aspeed2600-pwm-tacho.c | 756 > > +++ > > 3 files changed, 767 insertions(+) > > create mode 100644 drivers/hwmon/aspeed2600-pwm-tacho.c > > ... > > > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index > > 09a86c5e1d29..1a415d493ffc 100644 > > --- a/drivers/hwmon/Makefile > > +++ b/drivers/hwmon/Makefile > > @@ -52,6 +52,7 @@ obj-$(CONFIG_SENSORS_ARM_SCPI)+= > scpi-hwmon.o > > obj-$(CONFIG_SENSORS_AS370)+= as370-hwmon.o > > obj-$(CONFIG_SENSORS_ASC7621) += asc7621.o > > obj-$(CONFIG_SENSORS_ASPEED) += aspeed-pwm-tacho.o > > +obj-$(CONFIG_SENSORS_ASPEED2600_PWM_TACHO) += > aspeed2600-pwm-tacho.o > > Why does this have to be a separate implementation from the existing ASPEED > PWM/Tacho driver? Is there really nothing in common? > Hello Andrew, The register set is fully re-arrange. And it is new design at AST2600, can't be compatible with older PWM.
[PATCH v2] clk: aspeed: Fix APLL calculate formula from ast2600-A2
Starting from A2, the A-PLL calculation has changed. Use the
existing formula for A0/A1 and the new formula for A2 onwards.
Fixes: d3d04f6c330a ("clk: Add support for AST2600 SoC")
Signed-off-by: Ryan Chen
---
drivers/clk/clk-ast2600.c | 37 +++--
1 file changed, 27 insertions(+), 10 deletions(-)
diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c
index bbacaccad554..8933bd1506b3 100644
--- a/drivers/clk/clk-ast2600.c
+++ b/drivers/clk/clk-ast2600.c
@@ -17,7 +17,8 @@
#define ASPEED_G6_NUM_CLKS 71
-#define ASPEED_G6_SILICON_REV 0x004
+#define ASPEED_G6_SILICON_REV 0x014
+#define CHIP_REVISION_ID GENMASK(23, 16)
#define ASPEED_G6_RESET_CTRL 0x040
#define ASPEED_G6_RESET_CTRL2 0x050
@@ -190,18 +191,34 @@ static struct clk_hw *ast2600_calc_pll(const char *name,
u32 val)
static struct clk_hw *ast2600_calc_apll(const char *name, u32 val)
{
unsigned int mult, div;
+ u32 chip_id = readl(scu_g6_base + ASPEED_G6_SILICON_REV);
- if (val & BIT(20)) {
- /* Pass through mode */
- mult = div = 1;
+ if (((chip_id & CHIP_REVISION_ID) >> 16) >= 2) {
+ if (val & BIT(24)) {
+ /* Pass through mode */
+ mult = div = 1;
+ } else {
+ /* F = 25Mhz * [(m + 1) / (n + 1)] / (p + 1) */
+ u32 m = val & 0x1fff;
+ u32 n = (val >> 13) & 0x3f;
+ u32 p = (val >> 19) & 0xf;
+
+ mult = (m + 1);
+ div = (n + 1) * (p + 1);
+ }
} else {
- /* F = 25Mhz * (2-od) * [(m + 2) / (n + 1)] */
- u32 m = (val >> 5) & 0x3f;
- u32 od = (val >> 4) & 0x1;
- u32 n = val & 0xf;
+ if (val & BIT(20)) {
+ /* Pass through mode */
+ mult = div = 1;
+ } else {
+ /* F = 25Mhz * (2-od) * [(m + 2) / (n + 1)] */
+ u32 m = (val >> 5) & 0x3f;
+ u32 od = (val >> 4) & 0x1;
+ u32 n = val & 0xf;
- mult = (2 - od) * (m + 2);
- div = n + 1;
+ mult = (2 - od) * (m + 2);
+ div = n + 1;
+ }
}
return clk_hw_register_fixed_factor(NULL, name, "clkin", 0,
mult, div);
--
2.17.1
RE: [PATCH 1/1] clk: aspeed: Fix APLL calculate formula for ast2600-A2
> -Original Message-
> From: Joel Stanley
> Sent: Tuesday, January 19, 2021 11:10 AM
> To: Ryan Chen
> Cc: Joel Stanley ; Michael Turquette
> ; Stephen Boyd ;
> linux-...@vger.kernel.org; linux-kernel@vger.kernel.org;
> andre...@au1.ibm.com; BMC-SW ; Andrew
> Jeffery
> Subject: Re: [PATCH 1/1] clk: aspeed: Fix APLL calculate formula for
> ast2600-A2
>
> On Tue, 19 Jan 2021 at 03:04, Ryan Chen
> wrote:
> >
> > > -Original Message-
> > > From: Joel Stanley
> > > Sent: Tuesday, January 19, 2021 10:20 AM
> > > To: Ryan Chen ; Michael Turquette
> > > ; Stephen Boyd ;
> > > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > andre...@au1.ibm.com; BMC-SW
> > > Cc: j...@jms.id.au; Andrew Jeffery
> > > Subject: Re: [PATCH 1/1] clk: aspeed: Fix APLL calculate formula for
> > > ast2600-A2
> > >
> > > On Mon, 2021-01-18 at 18:08 +0800, Ryan Chen wrote:
> > > > AST2600A1/A2 have different pll calculate formula.
> > >
> > > To clarify, only the A0 has the old calculation, and all subsequent
> > > revisions use the new calculation?
> > >
> > > If this is the case, do we need to support A0 in mainline linux, or
> > > should we drop support for A0 and only support A1, A2 and onwards?
> > >
> > A0/A1 is use older calculate formula
> > After A2 is new calculate formula as the patch.
>
> Thanks for clarifying. I suggest you change the commit log to say something
> like this:
>
> Starting from A2, the A-PLL calculation has changed. Use the existing formula
> for A0/A1 and the new formula for A2 onwards.
>
> >
> > > You should add a line to indicate this is a fix:
> > >
> > > Fixes: d3d04f6c330a ("clk: Add support for AST2600 SoC")
> > >
> > Got it. so should I send new patch?
>
> Yes, please consider adjusting the commit message as I suggested above, and
> add the fixes line.
>
> > > > + u32 chip_id = readl(scu_g6_base + ASPEED_G6_SILICON_REV);
> > > >
> > > > - if (val & BIT(20)) {
> > > > - /* Pass through mode */
> > > > - mult = div = 1;
> > > > + if (((chip_id & CHIP_REVISION_ID) >> 16) >= 2) {
>
> Will this test be true if there are future versions of the chip (A3, etc)?
Yes, is also support for A3.
RE: [PATCH 1/1] clk: aspeed: Fix APLL calculate formula for ast2600-A2
> -Original Message-
> From: Joel Stanley
> Sent: Tuesday, January 19, 2021 10:20 AM
> To: Ryan Chen ; Michael Turquette
> ; Stephen Boyd ;
> linux-...@vger.kernel.org; linux-kernel@vger.kernel.org;
> andre...@au1.ibm.com; BMC-SW
> Cc: j...@jms.id.au; Andrew Jeffery
> Subject: Re: [PATCH 1/1] clk: aspeed: Fix APLL calculate formula for
> ast2600-A2
>
> On Mon, 2021-01-18 at 18:08 +0800, Ryan Chen wrote:
> > AST2600A1/A2 have different pll calculate formula.
>
> To clarify, only the A0 has the old calculation, and all subsequent revisions
> use
> the new calculation?
>
> If this is the case, do we need to support A0 in mainline linux, or should we
> drop support for A0 and only support A1, A2 and onwards?
>
A0/A1 is use older calculate formula
After A2 is new calculate formula as the patch.
> You should add a line to indicate this is a fix:
>
> Fixes: d3d04f6c330a ("clk: Add support for AST2600 SoC")
>
Got it. so should I send new patch?
> Also, when sending single patches you do not need to include the cover letter.
> You should include all of the relevant information in the patch's commit
> message.
>
> >
> > Signed-off-by: Ryan Chen
> > ---
> > drivers/clk/clk-ast2600.c | 37 +++--
> > 1 file changed, 27 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c
> > index bbacaccad554..8933bd1506b3 100644
> > --- a/drivers/clk/clk-ast2600.c
> > +++ b/drivers/clk/clk-ast2600.c
> > @@ -17,7 +17,8 @@
> >
> > #define ASPEED_G6_NUM_CLKS 71
> >
> > -#define ASPEED_G6_SILICON_REV 0x004
> > +#define ASPEED_G6_SILICON_REV 0x014 #define
> CHIP_REVISION_ID
> > +GENMASK(23, 16)
> >
> > #define ASPEED_G6_RESET_CTRL 0x040
> > #define ASPEED_G6_RESET_CTRL2 0x050 @@ -190,18
> +191,34 @@
> > static struct clk_hw *ast2600_calc_pll(const char *name, u32 val)
> > static struct clk_hw *ast2600_calc_apll(const char *name, u32 val)
> > {
> > unsigned int mult, div;
> > + u32 chip_id = readl(scu_g6_base + ASPEED_G6_SILICON_REV);
> >
> > - if (val & BIT(20)) {
> > - /* Pass through mode */
> > - mult = div = 1;
> > + if (((chip_id & CHIP_REVISION_ID) >> 16) >= 2) {
> > + if (val & BIT(24)) {
> > + /* Pass through mode */
> > + mult = div = 1;
> > + } else {
> > + /* F = 25Mhz * [(m + 1) / (n + 1)] / (p +
> 1)
> > */
> > + u32 m = val & 0x1fff;
> > + u32 n = (val >> 13) & 0x3f;
> > + u32 p = (val >> 19) & 0xf;
> > +
> > + mult = (m + 1);
> > + div = (n + 1) * (p + 1);
> > + }
> > } else {
> > - /* F = 25Mhz * (2-od) * [(m + 2) / (n + 1)] */
> > - u32 m = (val >> 5) & 0x3f;
> > - u32 od = (val >> 4) & 0x1;
> > - u32 n = val & 0xf;
> > + if (val & BIT(20)) {
> > + /* Pass through mode */
> > + mult = div = 1;
> > + } else {
> > + /* F = 25Mhz * (2-od) * [(m + 2) / (n +
> 1)]
> > */
> > + u32 m = (val >> 5) & 0x3f;
> > + u32 od = (val >> 4) & 0x1;
> > + u32 n = val & 0xf;
> >
> > - mult = (2 - od) * (m + 2);
> > - div = n + 1;
> > + mult = (2 - od) * (m + 2);
> > + div = n + 1;
> > + }
> > }
> > return clk_hw_register_fixed_factor(NULL, name, "clkin", 0,
> > mult, div);
>
[PATCH 1/1] clk: aspeed: Fix APLL calculate formula for ast2600-A2
AST2600A1/A2 have different pll calculate formula.
Signed-off-by: Ryan Chen
---
drivers/clk/clk-ast2600.c | 37 +++--
1 file changed, 27 insertions(+), 10 deletions(-)
diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c
index bbacaccad554..8933bd1506b3 100644
--- a/drivers/clk/clk-ast2600.c
+++ b/drivers/clk/clk-ast2600.c
@@ -17,7 +17,8 @@
#define ASPEED_G6_NUM_CLKS 71
-#define ASPEED_G6_SILICON_REV 0x004
+#define ASPEED_G6_SILICON_REV 0x014
+#define CHIP_REVISION_ID GENMASK(23, 16)
#define ASPEED_G6_RESET_CTRL 0x040
#define ASPEED_G6_RESET_CTRL2 0x050
@@ -190,18 +191,34 @@ static struct clk_hw *ast2600_calc_pll(const char *name,
u32 val)
static struct clk_hw *ast2600_calc_apll(const char *name, u32 val)
{
unsigned int mult, div;
+ u32 chip_id = readl(scu_g6_base + ASPEED_G6_SILICON_REV);
- if (val & BIT(20)) {
- /* Pass through mode */
- mult = div = 1;
+ if (((chip_id & CHIP_REVISION_ID) >> 16) >= 2) {
+ if (val & BIT(24)) {
+ /* Pass through mode */
+ mult = div = 1;
+ } else {
+ /* F = 25Mhz * [(m + 1) / (n + 1)] / (p + 1) */
+ u32 m = val & 0x1fff;
+ u32 n = (val >> 13) & 0x3f;
+ u32 p = (val >> 19) & 0xf;
+
+ mult = (m + 1);
+ div = (n + 1) * (p + 1);
+ }
} else {
- /* F = 25Mhz * (2-od) * [(m + 2) / (n + 1)] */
- u32 m = (val >> 5) & 0x3f;
- u32 od = (val >> 4) & 0x1;
- u32 n = val & 0xf;
+ if (val & BIT(20)) {
+ /* Pass through mode */
+ mult = div = 1;
+ } else {
+ /* F = 25Mhz * (2-od) * [(m + 2) / (n + 1)] */
+ u32 m = (val >> 5) & 0x3f;
+ u32 od = (val >> 4) & 0x1;
+ u32 n = val & 0xf;
- mult = (2 - od) * (m + 2);
- div = n + 1;
+ mult = (2 - od) * (m + 2);
+ div = n + 1;
+ }
}
return clk_hw_register_fixed_factor(NULL, name, "clkin", 0,
mult, div);
--
2.17.1
[PATCH 0/1] Fix AST2600A2 APLL calculate formuula
AST2600 A1/A2 have different pll, this patch fix for AST2600 A2 APLL calculate. Ryan Chen (1): clk: aspeed: Fix APLL calculate formula for ast2600-A2 drivers/clk/clk-ast2600.c | 37 +++-- 1 file changed, 27 insertions(+), 10 deletions(-) -- 2.17.1
RE: [PATCH v2 1/2] misc: Add clock control logic into Aspeed LPC SNOOP driver
> -Original Message- > From: Arnd Bergmann > Sent: Saturday, January 16, 2021 1:05 AM > To: Ryan Chen > Cc: John Wang ; > xuxiao...@bytedance.com; yulei...@bytedance.com; Robert Lippert > ; moderated list:ARM/ASPEED MACHINE SUPPORT > ; Greg Kroah-Hartman > ; Vernon Mauery > ; open list ; > Jae Hyun Yoo ; Patrick Venture > ; moderated list:ARM/ASPEED MACHINE SUPPORT > > Subject: Re: [PATCH v2 1/2] misc: Add clock control logic into Aspeed LPC > SNOOP driver > > On Wed, Jan 6, 2021 at 10:57 AM Ryan Chen > wrote: > > > > Hello John, Joel, Jae, > > For this should be set LCLK to be CRITICAL it will fix LPC related > driver. (KCS/BT/SNOOP) > > I have send the patch before. > > > > https://patchwork.ozlabs.org/project/linux-aspeed/patch/20200928070108 > > .14040-2-ryan_c...@aspeedtech.com/ > > > > Hello Joel, > > Will you consider this patch? > > Beside KCS/BT/SNOOP, UART1/UART2 will be most related issue > for host side. > > Sorry it did not make it into the merge window. The patch is still in > patchwork. > I could just pick it up directly for v5.12, or wait for a combined pull > request > with other work. Hello Arnd, Thanks your update. >Joel, please let me know what you prefer. > Hello Joel, Could you help check on this patch? https://patchwork.ozlabs.org/project/linux-aspeed/patch/20200928070108.14040-2-ryan_c...@aspeedtech.com/
Re: [PATCH] Adds a new ioctl32 syscall for backwards compatibility layers
On Wed, Jan 6, 2021 at 12:49 AM Arnd Bergmann wrote: > > On Wed, Jan 6, 2021 at 7:48 AM wrote: > > From: Ryan Houdek > ... > > This does not solve the following problems: > > 1) compat_alloc_user_space inside ioctl > > 2) ioctls that check task mode instead of entry point for behaviour > > 3) ioctls allocating memory > > 4) struct packing problems between architectures > > > > Workarounds for the problems presented: > > 1a) Do a stack pivot to the lower 32bits from userspace > > - Forces host 64bit process to have its thread stacks to live in 32bit > > space. Not ideal. > > - Only do a stack pivot on ioctl to save previous 32bit VA space > > 1b) Teach kernel that compat_alloc_userspace can return a 64bit pointer > > - x86-64 truncates stack from this function > > - AArch64 returns the full stack pointer > > - Only ~29 users. Validating all of them support a 64bit stack is > > trivial? > > I've almost completed the removal of compat_alloc_user_space(), > that should no longer be a concern when the syscall gets added. > > > 2a) Any application using these can be checked for compatibility in > > userspace and put on a block list. > > 2b) Fix any ioctls doing broken behaviour based on task mode rather than > > ioctl entry point > > What the ioctls() actually check is 'in_compat_syscall()', which is not > the mode of the task but the type of syscall. There is actually a general > trend to use this helper more rather than less, and I think the only > way forward here is to ensure that this returns true when entering > through the new syscall number. > > For x86, this has another complication, as some ioctls also need to > check whether they are in an ia32 task (with packed u64 and 32-bit > __kernel_old_time_t) or an x32 task (with aligned u64 and 64-bit > __kernel_old_time_t). If the new syscall gets wired up on x86 as well, > you'd need to decide which of the two behaviors you want. I can have a follow-up patch that makes this do ni_syscall on x86_64 since we can go through the int 0x80 handler, or x32 handler path and choose whichever one there. > > > 3a) Userspace consumes all VA space above 32bit. Forcing allocations to > > occur in lower 32bits > > - This is the current implementation > > 3b) Ensure any allocation in the ioctl handles ioctl entrypoint rather > > than just allow generic memory allocations in full VA space > > - This is hard to guarantee > > What kind of allocation do you mean here? Can you give an example of > an ioctl that does this? My concern here would be something like DRM allocating memory and returning a pointer to userspace that ends up in 64bit space. I can see something like `drm_get_unmapped_area` calls in to `current->mm->get_unmapped_area` which I believe only ends up falling down TASK_SIZE checks. Which could potentially return pointers in the 64bit address space range in this case. Theoretically can be resolved either by thieving the full 64bit VA range, or doing something like the Tango layer patches that on syscall entry changes the syscall to a "compat" syscall. compat syscall flag like Tango might be nicer here? > > > 4a) Blocklist any application using ioctls that have different struct > > packing across the boundary > > - Can happen when struct packing of 32bit x86 application goes down > > the aarch64 compat_ioctl path > > - Userspace is a AArch64 process passing 32bit x86 ioctl structures > > through the compat_ioctl path which is typically for AArch32 processes > > - None currently identified > > 4b) Work with upstream kernel and userspace projects to evaluate and fix > > - Identify the problem ioctls > > - Implement a new ioctl with more sane struct packing that matches > > cross-arch > > - Implement new ioctl while maintaining backwards compatibility with > > previous ioctl handler > > - Change upstream project to use the new compatibility ioctl > > - ioctl deprecation will be case by case per device and project > > 4b) Userspace implements a full ioctl emulation layer > > - Parses the full ioctl tree > > - Either passes through ioctls that it doesn't understand or > > transforms ioctls that it knows are trouble > > - Has the downside that it can still run in to edge cases that will > > fail > > - Performance of additional tracking is a concern > > - Prone to failure keeping the kernel ioctl and userspace ioctl > > handling in sync > > - Really want to have it in the kernel space as much as possible > > I think there are only a few ioctls that are affected, and you can > probably get a list fro
[PATCH 0/1] Fix vhub engine stop dma register setting.
This patch fixes vhub engine stop dma should have different register setting. Ryan Chen (1): usb: gadget: aspeed: fix stop dma register setting. drivers/usb/gadget/udc/aspeed-vhub/epn.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) -- 2.17.1
[PATCH 1/1] usb: gadget: aspeed: fix stop dma register setting.
The vhub engine has two dma mode, one is descriptor list, another
is single stage DMA. Each mode has different stop register setting.
Descriptor list operation (bit2) : 0 disable reset, 1: enable reset
Single mode operation (bit0) : 0 : disable, 1: enable
Signed-off-by: Ryan Chen
---
drivers/usb/gadget/udc/aspeed-vhub/epn.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
index 0bd6b20435b8..02d8bfae58fb 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
@@ -420,7 +420,10 @@ static void ast_vhub_stop_active_req(struct ast_vhub_ep
*ep,
u32 state, reg, loops;
/* Stop DMA activity */
- writel(0, ep->epn.regs + AST_VHUB_EP_DMA_CTLSTAT);
+ if (ep->epn.desc_mode)
+ writel(VHUB_EP_DMA_CTRL_RESET, ep->epn.regs +
AST_VHUB_EP_DMA_CTLSTAT);
+ else
+ writel(0, ep->epn.regs + AST_VHUB_EP_DMA_CTLSTAT);
/* Wait for it to complete */
for (loops = 0; loops < 1000; loops++) {
--
2.17.1
RE: [PATCH 5/6] soc: aspeed: Add eSPI driver
> -Original Message- > From: Joel Stanley > Sent: Friday, January 8, 2021 10:59 AM > To: ChiaWei Wang ; Jeremy Kerr > > Cc: Rob Herring ; and...@aj.id.au; t...@linutronix.de; > m...@kernel.org; p.za...@pengutronix.de; linux-asp...@lists.ozlabs.org; > open...@lists.ozlabs.org; devicet...@vger.kernel.org; > linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; BMC-SW > > Subject: Re: [PATCH 5/6] soc: aspeed: Add eSPI driver > > On Thu, 7 Jan 2021 at 02:39, ChiaWei Wang > wrote: > > > > Hi Rob, > > > > > -Original Message- > > > From: Rob Herring > > > Sent: Wednesday, January 6, 2021 11:32 PM > > > To: ChiaWei Wang > > > Subject: Re: [PATCH 5/6] soc: aspeed: Add eSPI driver > > > > > > On Wed, Jan 06, 2021 at 01:59:38PM +0800, Chia-Wei, Wang wrote: > > > > The Aspeed eSPI controller is slave device to communicate with the > > > > master through the Enhanced Serial Peripheral Interface (eSPI). > > > > All of the four eSPI channels, namely peripheral, virtual wire, > > > > out-of-band, and flash are supported. > > > > > > > > Signed-off-by: Chia-Wei, Wang > > > > --- > > > > drivers/soc/aspeed/Kconfig | 49 ++ > > > > drivers/soc/aspeed/Makefile | 5 + > > > > drivers/soc/aspeed/aspeed-espi-ctrl.c | 197 ++ > > > > drivers/soc/aspeed/aspeed-espi-flash.c | 490 ++ > > > > drivers/soc/aspeed/aspeed-espi-oob.c| 706 > > > > > > > drivers/soc/aspeed/aspeed-espi-peripheral.c | 613 > + > > > > drivers/soc/aspeed/aspeed-espi-vw.c | 211 ++ > > > > include/uapi/linux/aspeed-espi.h| 160 + > > > > 8 files changed, 2431 insertions(+) create mode 100644 > > > > drivers/soc/aspeed/aspeed-espi-ctrl.c > > > > create mode 100644 drivers/soc/aspeed/aspeed-espi-flash.c > > > > create mode 100644 drivers/soc/aspeed/aspeed-espi-oob.c > > > > create mode 100644 drivers/soc/aspeed/aspeed-espi-peripheral.c > > > > create mode 100644 drivers/soc/aspeed/aspeed-espi-vw.c > > > > > > drivers/spi/ is the correct location for a SPI controller. > > > > > > > create mode 100644 include/uapi/linux/aspeed-espi.h > > > > > > This userspace interface is not going to be accepted upstream. > > > > > > I'd suggest you look at similar SPI flash capable SPI controller > > > drivers upstream and model your driver after them. This looks like it > > > needs > major reworking. > > > > > eSPI resues the timing and electrical specification of SPI but runs > > completely > different protocol. > > Only the flash channel is related to SPI and the other 3 channels are for > EC/BMC/SIO. > > Therefore, an eSPI driver might not fit into the SPI model. > > I agree, the naming is confusing but eSPI doesn't belong in drivers/spi. > > As it is a bus that is common to more than just the Aspeed BMC, we may want > to implement it as a new bus type that has devices hanging off it, similar to > FSI. > The ASPEED eSPI controller driver is slave side device, not master side. I think it will be stay soc/aspeed first. Because is most SoC Chip related. Cheers, Ryan
RE: [PATCH v2 1/2] misc: Add clock control logic into Aspeed LPC SNOOP driver
Hello John, Joel, Jae,
For this should be set LCLK to be CRITICAL it will fix LPC related
driver. (KCS/BT/SNOOP)
I have send the patch before.
https://patchwork.ozlabs.org/project/linux-aspeed/patch/20200928070108.14040-2-ryan_c...@aspeedtech.com/
Hello Joel,
Will you consider this patch?
Beside KCS/BT/SNOOP, UART1/UART2 will be most related issue for host
side.
> -Original Message-
> From: Linux-aspeed
> On
> Behalf Of John Wang
> Sent: Tuesday, December 8, 2020 5:18 PM
> To: xuxiao...@bytedance.com; yulei...@bytedance.com
> Cc: Robert Lippert ; moderated list:ARM/ASPEED
> MACHINE SUPPORT ; Greg Kroah-Hartman
> ; Vernon Mauery
> ; open list ;
> Jae Hyun Yoo ; Patrick Venture
> ; moderated list:ARM/ASPEED MACHINE SUPPORT
>
> Subject: [PATCH v2 1/2] misc: Add clock control logic into Aspeed LPC SNOOP
> driver
>
> From: Jae Hyun Yoo
>
> If LPC SNOOP driver is registered ahead of lpc-ctrl module, LPC SNOOP block
> will be enabled without heart beating of LCLK until lpc-ctrl enables the LCLK.
> This issue causes improper handling on host interrupts when the host sends
> interrupt in that time frame.
> Then kernel eventually forcibly disables the interrupt with dumping stack and
> printing a 'nobody cared this irq' message out.
>
> To prevent this issue, all LPC sub-nodes should enable LCLK individually so
> this
> patch adds clock control logic into the LPC SNOOP driver.
>
> Fixes: 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc
> chardev")
>
> Signed-off-by: Jae Hyun Yoo
> Signed-off-by: Vernon Mauery
> Signed-off-by: John Wang
> ---
> v2:
> reword: Add fixes line
> ---
> drivers/soc/aspeed/aspeed-lpc-snoop.c | 30 ---
> 1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> index 682ba0eb4eba..20acac6342ef 100644
> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> @@ -11,6 +11,7 @@
> */
>
> #include
> +#include
> #include
> #include
> #include
> @@ -67,6 +68,7 @@ struct aspeed_lpc_snoop_channel { struct
> aspeed_lpc_snoop {
> struct regmap *regmap;
> int irq;
> + struct clk *clk;
> struct aspeed_lpc_snoop_channel chan[NUM_SNOOP_CHANNELS]; };
>
> @@ -282,22 +284,42 @@ static int aspeed_lpc_snoop_probe(struct
> platform_device *pdev)
> return -ENODEV;
> }
>
> + lpc_snoop->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(lpc_snoop->clk)) {
> + rc = PTR_ERR(lpc_snoop->clk);
> + if (rc != -EPROBE_DEFER)
> + dev_err(dev, "couldn't get clock\n");
> + return rc;
> + }
> + rc = clk_prepare_enable(lpc_snoop->clk);
> + if (rc) {
> + dev_err(dev, "couldn't enable clock\n");
> + return rc;
> + }
> +
> rc = aspeed_lpc_snoop_config_irq(lpc_snoop, pdev);
> if (rc)
> - return rc;
> + goto err;
>
> rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, 0, port);
> if (rc)
> - return rc;
> + goto err;
>
> /* Configuration of 2nd snoop channel port is optional */
> if (of_property_read_u32_index(dev->of_node, "snoop-ports",
> 1, ) == 0) {
> rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, 1, port);
> - if (rc)
> + if (rc) {
> aspeed_lpc_disable_snoop(lpc_snoop, 0);
> + goto err;
> + }
> }
>
> + return 0;
> +
> +err:
> + clk_disable_unprepare(lpc_snoop->clk);
> +
> return rc;
> }
>
> @@ -309,6 +331,8 @@ static int aspeed_lpc_snoop_remove(struct
> platform_device *pdev)
> aspeed_lpc_disable_snoop(lpc_snoop, 0);
> aspeed_lpc_disable_snoop(lpc_snoop, 1);
>
> + clk_disable_unprepare(lpc_snoop->clk);
> +
> return 0;
> }
>
> --
> 2.25.1
RE: [PATCH 2/3] aspeed-video: clear spurious interrupt bits unconditionally
> -Original Message- > From: Zev Weiss > Sent: Wednesday, December 23, 2020 11:54 AM > To: Ryan Chen > Cc: Joel Stanley ; Eddie James ; > Mauro Carvalho Chehab ; Andrew Jeffery > ; linux-me...@vger.kernel.org; OpenBMC Maillist > ; Linux ARM > ; linux-aspeed > ; Linux Kernel Mailing List > ; Jae Hyun Yoo > Subject: Re: [PATCH 2/3] aspeed-video: clear spurious interrupt bits > unconditionally > > On Tue, Dec 22, 2020 at 08:53:33PM CST, Ryan Chen wrote: > >> -Original Message- > >> From: Joel Stanley > >> Sent: Wednesday, December 23, 2020 9:07 AM > >> To: Zev Weiss ; Ryan Chen > >> > >> Cc: Eddie James ; Mauro Carvalho Chehab > >> ; Andrew Jeffery ; > >> linux-me...@vger.kernel.org; OpenBMC Maillist > >> ; Linux ARM > >> ; linux-aspeed > >> ; Linux Kernel Mailing List > >> ; Jae Hyun Yoo > >> > >> Subject: Re: [PATCH 2/3] aspeed-video: clear spurious interrupt bits > >> unconditionally > >> > >> On Tue, 22 Dec 2020 at 19:14, Zev Weiss wrote: > >> > > >> > On Mon, Dec 21, 2020 at 10:47:37PM CST, Joel Stanley wrote: > >> > >On Tue, 15 Dec 2020 at 02:46, Zev Weiss > wrote: > >> > >> > >> > >> Instead of testing and conditionally clearing them one by one, > >> > >> we can instead just unconditionally clear them all at once. > >> > >> > >> > >> Signed-off-by: Zev Weiss > >> > > > >> > >I had a poke at the assembly and it looks like GCC is clearing the > >> > >bits unconditionally anyway, so removing the tests provides no change. > >> > > > >> > >Combining them is a good further optimization. > >> > > > >> > >Reviewed-by: Joel Stanley > >> > > > >> > >A question unrelated to this patch: Do you know why the driver > >> > >doesn't clear the status bits in the interrupt handler? I would > >> > >expect it to write the value of sts back to the register to ack > >> > >the pending interrupt. > >> > > > >> > > >> > No, I don't, and I was sort of wondering the same thing actually -- > >> > I'm not deeply familiar with this hardware or driver though, so I > >> > was a bit hesitant to start messing with things. (Though maybe > >> > doing so would address the "stickiness" aspect when it does > >> > manifest.) Perhaps Eddie or Jae can shed some light here? > >> > >> I think you're onto something here - this would be why the status > >> bits seem to stick until the device is reset. > >> > >> Until Aspeed can clarify if this is a hardware or software issue, I > >> suggest we ack the bits and log a message when we see them, instead > >> of always ignoring them without taking any action. > >> > >> Can you write a patch that changes the interrupt handler to ack > >> status bits as it handles each of them? > >> > >Hello Zev, before the patch, do you met issue with irq handler? > >[continuous incoming?] > > > >In aspeed_video_irq handler should only handle enable interrupt expected. > > u32 sts = aspeed_video_read(video, VE_INTERRUPT_STATUS); > > + sts &= aspeed_video_read(video, VE_INTERRUPT_CTRL); > > > >Ryan > > > > Hi Ryan, > > Prior to any of these patches I encountered a problem pretty much exactly like > what Jae described in his commit message in 65d270acb2d (but the kernel I > was running included that patch). Adding the diagnostic in patch #1 of this > series showed that it was apparently the same problem, just with a different > interrupt that Jae's patch didn't include. > > From what you wrote above, I gather that it is in fact expected for the > hardware to assert interrupts that aren't enabled in VE_INTERRUPT_CTRL? > If so, I guess something like that would obviate the need for both Jae's > earlier > patch and this whole series. > Yes, I expected handle enabled in VE_INTERRUPT_CTRL. > I think the question Joel raised is somewhat independent though -- if the > VE_INTERRUPT_STATUS register asserts interrupts we're not actually using, > should the driver acknowledge them anyway or just leave them alone? My opinion will keep them alone, ignore them. > (Though if we're just going to ignore them anyway maybe it doesn't ultimately > matter very much.) > > > Zev
RE: [PATCH 2/3] aspeed-video: clear spurious interrupt bits unconditionally
> -Original Message- > From: Joel Stanley > Sent: Wednesday, December 23, 2020 9:07 AM > To: Zev Weiss ; Ryan Chen > > Cc: Eddie James ; Mauro Carvalho Chehab > ; Andrew Jeffery ; > linux-me...@vger.kernel.org; OpenBMC Maillist ; > Linux ARM ; linux-aspeed > ; Linux Kernel Mailing List > ; Jae Hyun Yoo > Subject: Re: [PATCH 2/3] aspeed-video: clear spurious interrupt bits > unconditionally > > On Tue, 22 Dec 2020 at 19:14, Zev Weiss wrote: > > > > On Mon, Dec 21, 2020 at 10:47:37PM CST, Joel Stanley wrote: > > >On Tue, 15 Dec 2020 at 02:46, Zev Weiss wrote: > > >> > > >> Instead of testing and conditionally clearing them one by one, we > > >> can instead just unconditionally clear them all at once. > > >> > > >> Signed-off-by: Zev Weiss > > > > > >I had a poke at the assembly and it looks like GCC is clearing the > > >bits unconditionally anyway, so removing the tests provides no change. > > > > > >Combining them is a good further optimization. > > > > > >Reviewed-by: Joel Stanley > > > > > >A question unrelated to this patch: Do you know why the driver > > >doesn't clear the status bits in the interrupt handler? I would > > >expect it to write the value of sts back to the register to ack the > > >pending interrupt. > > > > > > > No, I don't, and I was sort of wondering the same thing actually -- > > I'm not deeply familiar with this hardware or driver though, so I was > > a bit hesitant to start messing with things. (Though maybe doing so > > would address the "stickiness" aspect when it does manifest.) Perhaps > > Eddie or Jae can shed some light here? > > I think you're onto something here - this would be why the status bits seem to > stick until the device is reset. > > Until Aspeed can clarify if this is a hardware or software issue, I suggest > we ack > the bits and log a message when we see them, instead of always ignoring them > without taking any action. > > Can you write a patch that changes the interrupt handler to ack status bits > as it > handles each of them? > Hello Zev, before the patch, do you met issue with irq handler? [continuous incoming?] In aspeed_video_irq handler should only handle enable interrupt expected. u32 sts = aspeed_video_read(video, VE_INTERRUPT_STATUS); + sts &= aspeed_video_read(video, VE_INTERRUPT_CTRL); Ryan > > > > > > Zev > >
RE: [PATCH v2 3/3] ARM: dts: add ehci uhci enable in evb dts
> -Original Message- > From: Andrew Jeffery > Sent: Thursday, October 22, 2020 7:45 AM > To: Ryan Chen ; Sergei Shtylyov > ; Joel Stanley ; > linux-arm-ker...@lists.infradead.org; linux-asp...@lists.ozlabs.org; > linux-kernel@vger.kernel.org; linux-...@vger.kernel.org; BMC-SW > ; Alan Stern > Subject: Re: [PATCH v2 3/3] ARM: dts: add ehci uhci enable in evb dts > > Hi Ryan, > > On Mon, 12 Oct 2020, at 11:13, Ryan Chen wrote: > > Hello Segei, > > > > > -Original Message- > > > From: Sergei Shtylyov > > > Sent: Friday, October 9, 2020 4:23 PM > > > To: Ryan Chen ; Joel Stanley > > > ; Andrew Jeffery ; > > > linux-arm-ker...@lists.infradead.org; > > > linux-asp...@lists.ozlabs.org; linux-kernel@vger.kernel.org; > > > linux-...@vger.kernel.org; BMC-SW ; Alan > > > Stern > > > Subject: Re: [PATCH v2 3/3] ARM: dts: add ehci uhci enable in evb > > > dts > > > > > > HEllo! > > > > > > On 09.10.2020 5:49, Ryan Chen wrote: > > > > > > > Add EHCI UHCI enable build in aspeed-ast2600-evb.dts > > > > > > Enable ECHI/UHCI for the Aspeed AST2600 EVB, perhaps? > > > > > > > Yes, it is enable for AST2600 EVB. > > I think Sergei was suggesting you change the wording of the commit message. > Thanks, will resend the patch with modification.
RE: [PATCH 1/1] clk: aspeed: modify some default clks are critical
> -Original Message- > From: Joel Stanley > Sent: Wednesday, October 14, 2020 1:28 PM > To: Stephen Boyd > Cc: Andrew Jeffery ; Michael Turquette > ; Ryan Chen ; > BMC-SW ; Linux ARM > ; linux-aspeed > ; linux-...@vger.kernel.org; Linux Kernel > Mailing List > Subject: Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical > > On Wed, 14 Oct 2020 at 02:50, Stephen Boyd wrote: > > > > Quoting Ryan Chen (2020-09-28 00:01:08) > > > In ASPEED SoC LCLK is LPC clock for all SuperIO device, UART1/UART2 > > > are default for Host SuperIO UART device, eSPI clk for Host eSPI bus > > > access eSPI slave channel, those clks can't be disable should keep > > > default, otherwise will affect Host side access SuperIO and SPI slave > > > device. > > > > > > Signed-off-by: Ryan Chen > > > --- > > > > Is there resolution on this thread? > > Not yet. > > We have a system where the BMC (management controller) controls some > clocks, but the peripherals that it's clocking are outside the BMC's control. > In > this case, the host processor us using some UARTs and what not independent of > any code running on the BMC. > > Ryan wants to have them marked as critical so the BMC never powers them > down. > > However, there are systems that don't use this part of the soc, so for those > implementations they are not critical and Linux on the BMC can turn them off. > Take an example, conflict thought about ASPEED_CLK_GATE_BCLK is CLK_IS_CRITICAL in clk-ast2600.c In my opinion, the driver should keep the SoC default clk setting. It is original chip feature. > Do you have any thoughts? Has anyone solved a similar problem already? >
RE: [PATCH v2 3/3] ARM: dts: add ehci uhci enable in evb dts
Hello Segei, > -Original Message- > From: Sergei Shtylyov > Sent: Friday, October 9, 2020 4:23 PM > To: Ryan Chen ; Joel Stanley ; > Andrew Jeffery ; linux-arm-ker...@lists.infradead.org; > linux-asp...@lists.ozlabs.org; linux-kernel@vger.kernel.org; > linux-...@vger.kernel.org; BMC-SW ; Alan Stern > > Subject: Re: [PATCH v2 3/3] ARM: dts: add ehci uhci enable in evb dts > > HEllo! > > On 09.10.2020 5:49, Ryan Chen wrote: > > > Add EHCI UHCI enable build in aspeed-ast2600-evb.dts > > Enable ECHI/UHCI for the Aspeed AST2600 EVB, perhaps? > Yes, it is enable for AST2600 EVB. > > Signed-off-by: Ryan Chen > [...] > > MBR, Sergei
[PATCH] risc-v: kernel: ftrace: Fixes improper SPDX comment style
Signed-off-by: Ryan Kosta --- arch/riscv/kernel/ftrace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c index 99e12faa549..765b62434f3 100644 --- a/arch/riscv/kernel/ftrace.c +++ b/arch/riscv/kernel/ftrace.c @@ -1,4 +1,4 @@ -/* SPDX-License-Identifier: GPL-2.0 */ +// SPDX-License-Identifier: GPL-2.0 /* * Copyright (C) 2013 Linaro Limited * Author: AKASHI Takahiro -- 2.20.1
RE: [PATCH v2 1/3] configs: aspeed: enable UHCI driver in defconfig
> -Original Message- > From: Joel Stanley > Sent: Friday, October 9, 2020 12:55 PM > To: Greg KH > Cc: Ryan Chen ; Andrew Jeffery > ; Linux ARM ; > linux-aspeed ; Linux Kernel Mailing List > ; linux-...@vger.kernel.org; BMC-SW > ; Alan Stern > Subject: Re: [PATCH v2 1/3] configs: aspeed: enable UHCI driver in defconfig > > On Fri, 9 Oct 2020 at 04:45, Greg KH wrote: > > > > On Fri, Oct 09, 2020 at 10:49:35AM +0800, Ryan Chen wrote: > > > v2: > > > -Changed : Add SCSI, BLK_DEV_SD, USB_STORAGE support. > > > v1: > > > -Enable UHCI driver in aspeed_g5_defconfig. > > > > > > Signed-off-by: Ryan Chen > > > > Why do you need this in a defconfig? > > I would prefer configurations that are being used to be present in the > defconfig > so we can test it. I think this is a sensible change. > > Ryan, I gave you my Reviewed-by for the last version of the patch. As you did > not change the contents of this patch it is fine for you to leave my > Reviewed-by > on it. Joel, thanks the review, so I need add Reviewed-by at each patch if someone have reviewed the previous patch, am I right?
RE: [PATCH v2 1/3] configs: aspeed: enable UHCI driver in defconfig
> -Original Message- > From: Greg KH > Sent: Friday, October 9, 2020 1:47 PM > To: Joel Stanley > Cc: Ryan Chen ; Andrew Jeffery > ; Linux ARM ; > linux-aspeed ; Linux Kernel Mailing List > ; linux-...@vger.kernel.org; BMC-SW > ; Alan Stern > Subject: Re: [PATCH v2 1/3] configs: aspeed: enable UHCI driver in defconfig > > On Fri, Oct 09, 2020 at 04:55:19AM +, Joel Stanley wrote: > > On Fri, 9 Oct 2020 at 04:45, Greg KH wrote: > > > > > > On Fri, Oct 09, 2020 at 10:49:35AM +0800, Ryan Chen wrote: > > > > v2: > > > > -Changed : Add SCSI, BLK_DEV_SD, USB_STORAGE support. > > > > v1: > > > > -Enable UHCI driver in aspeed_g5_defconfig. > > > > > > > > Signed-off-by: Ryan Chen > > > > > > Why do you need this in a defconfig? > > > > I would prefer configurations that are being used to be present in the > > defconfig so we can test it. I think this is a sensible change. > > Then it needs to be described in the changelog, otherwise we have no idea > why this is happening :) > Ok, will send new version describe it.
[PATCH v2 2/3] usb: host: add uhci compatible support for ast2600-uhci
v2:
- Fix continuation lines, align with "of_device"
v1:
- Add support for AST2600 SOC UHCI driver.
Signed-off-by: Ryan Chen
---
drivers/usb/host/uhci-platform.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/host/uhci-platform.c b/drivers/usb/host/uhci-platform.c
index 70dbd95c3f06..be9e9db7cad1 100644
--- a/drivers/usb/host/uhci-platform.c
+++ b/drivers/usb/host/uhci-platform.c
@@ -113,7 +113,8 @@ static int uhci_hcd_platform_probe(struct platform_device
*pdev)
num_ports);
}
if (of_device_is_compatible(np, "aspeed,ast2400-uhci") ||
- of_device_is_compatible(np, "aspeed,ast2500-uhci")) {
+ of_device_is_compatible(np, "aspeed,ast2500-uhci") ||
+ of_device_is_compatible(np, "aspeed,ast2600-uhci")) {
uhci->is_aspeed = 1;
dev_info(>dev,
"Enabled Aspeed implementation workarounds\n");
--
2.17.1
[PATCH v2 0/3] Enable USB host for AST2600
v2: -[1/3]: Add SCSI,BLK_DEV_SD,USB_STORAGE in defconfig. -[2/3]: Fix continuation lines. v1: -The patches enable UHCI driver in AST2600 and also enable USB host in aspeed-ast2600-evb.dts. Ryan Chen (3): configs: aspeed: enable UHCI driver in defconfig usb: host: add uhci compatible support for ast2600-uhci ARM: dts: add ehci uhci enable in evb dts arch/arm/boot/dts/aspeed-ast2600-evb.dts | 8 arch/arm/configs/aspeed_g5_defconfig | 4 drivers/usb/host/uhci-platform.c | 3 ++- 3 files changed, 14 insertions(+), 1 deletion(-) -- 2.17.1
[PATCH v2 3/3] ARM: dts: add ehci uhci enable in evb dts
Add EHCI UHCI enable build in aspeed-ast2600-evb.dts
Signed-off-by: Ryan Chen
---
arch/arm/boot/dts/aspeed-ast2600-evb.dts | 8
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
index 89be13197780..2772796e215e 100644
--- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
+++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
@@ -237,3 +237,11 @@
{
status = "okay";
};
+
+ {
+ status = "okay";
+};
+
+ {
+ status = "okay";
+};
--
2.17.1
[PATCH v2 1/3] configs: aspeed: enable UHCI driver in defconfig
v2: -Changed : Add SCSI, BLK_DEV_SD, USB_STORAGE support. v1: -Enable UHCI driver in aspeed_g5_defconfig. Signed-off-by: Ryan Chen --- arch/arm/configs/aspeed_g5_defconfig | 4 1 file changed, 4 insertions(+) diff --git a/arch/arm/configs/aspeed_g5_defconfig b/arch/arm/configs/aspeed_g5_defconfig index 2bacd8c90f4b..7a6b0b79be9f 100644 --- a/arch/arm/configs/aspeed_g5_defconfig +++ b/arch/arm/configs/aspeed_g5_defconfig @@ -90,6 +90,8 @@ CONFIG_BLK_DEV_NBD=y CONFIG_MCTP_LPC=y CONFIG_EEPROM_AT24=y CONFIG_EEPROM_AT25=y +CONFIG_SCSI=y +CONFIG_BLK_DEV_SD=y CONFIG_MD=y CONFIG_BLK_DEV_DM=y CONFIG_DM_VERITY=y @@ -212,6 +214,8 @@ CONFIG_USB_DYNAMIC_MINORS=y CONFIG_USB_EHCI_HCD=y CONFIG_USB_EHCI_ROOT_HUB_TT=y CONFIG_USB_EHCI_HCD_PLATFORM=y +CONFIG_USB_UHCI_HCD=y +CONFIG_USB_STORAGE=y CONFIG_USB_GADGET=y CONFIG_USB_ASPEED_VHUB=y CONFIG_USB_CONFIGFS=y -- 2.17.1
RE: [PATCH 1/1] clk: aspeed: modify some default clks are critical
> -Original Message-
> From: Joel Stanley
> Sent: Wednesday, October 7, 2020 7:34 PM
> To: Ryan Chen
> Cc: Jae Hyun Yoo ; Andrew Jeffery
> ; Michael Turquette ; Stephen
> Boyd ; linux-...@vger.kernel.org; Linux ARM
> ; linux-aspeed
> ; Linux Kernel Mailing List
> ; BMC-SW
> Subject: Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical
>
> On Tue, 29 Sep 2020 at 08:40, Ryan Chen
> wrote:
> >
> > > From: Joel Stanley
> > > Sent: Tuesday, September 29, 2020 4:04 PM
> > > To: Ryan Chen ; Jae Hyun Yoo
> > > ; Andrew Jeffery
> > > Cc: Michael Turquette ; Stephen Boyd
> > > ; linux-...@vger.kernel.org; Linux ARM
> > > ; linux-aspeed
> > > ; Linux Kernel Mailing List
> > > ; BMC-SW
> > > Subject: Re: [PATCH 1/1] clk: aspeed: modify some default clks are
> > > critical
> > >
> > > On Mon, 28 Sep 2020 at 07:01, Ryan Chen
> > > wrote:
> > > >
> > > > In ASPEED SoC LCLK is LPC clock for all SuperIO device,
> > > > UART1/UART2 are default for Host SuperIO UART device, eSPI clk for
> > > > Host eSPI bus access eSPI slave channel, those clks can't be
> > > > disable should keep default, otherwise will affect Host side access
> SuperIO and SPI slave device.
> > > >
> > > > Signed-off-by: Ryan Chen
> > > > ---
> > > > drivers/clk/clk-aspeed.c | 8 drivers/clk/clk-ast2600.c
> > > > | 8
> > > >
> > > > 2 files changed, 8 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> > > > index 411ff5fb2c07..d348c4fd3f9f 100644
> > > > --- a/drivers/clk/clk-aspeed.c
> > > > +++ b/drivers/clk/clk-aspeed.c
> > > > @@ -54,15 +54,15 @@ static const struct aspeed_gate_data
> > > > aspeed_gates[]
> > > = {
> > > > [ASPEED_CLK_GATE_DCLK] ={ 5, -1, "dclk-gate",
> > > NULL, CLK_IS_CRITICAL }, /* DAC */
> > > > [ASPEED_CLK_GATE_REFCLK] = { 6, -1, "refclk-gate",
> > > "clkin", CLK_IS_CRITICAL },
> > > > [ASPEED_CLK_GATE_USBPORT2CLK] = { 7, 3,
> > > > "usb-port2-gate",
> > > NULL, 0 }, /* USB2.0 Host port 2 */
> > > > - [ASPEED_CLK_GATE_LCLK] ={ 8, 5, "lclk-gate",
> > > NULL, 0 }, /* LPC */
> > > > + [ASPEED_CLK_GATE_LCLK] ={ 8, 5, "lclk-gate",
> > > NULL, CLK_IS_CRITICAL }, /* LPC */
> > > > [ASPEED_CLK_GATE_USBUHCICLK] = { 9, 15,
> "usb-uhci-gate",
> > > NULL, 0 }, /* USB1.1 (requires port 2 enabled) */
> > > > [ASPEED_CLK_GATE_D1CLK] = { 10, 13, "d1clk-gate",
> > > NULL, 0 }, /* GFX CRT */
> > > > [ASPEED_CLK_GATE_YCLK] ={ 13, 4, "yclk-gate",
> > > NULL, 0 }, /* HAC */
> > > > [ASPEED_CLK_GATE_USBPORT1CLK] = { 14, 14,
> > > > "usb-port1-gate",
> > > NULL, 0 }, /* USB2 hub/USB2 host port 1/USB1.1 dev */
> > > > - [ASPEED_CLK_GATE_UART1CLK] ={ 15, -1, "uart1clk-gate",
> > > "uart", 0 }, /* UART1 */
> > > > - [ASPEED_CLK_GATE_UART2CLK] ={ 16, -1, "uart2clk-gate",
> > > "uart", 0 }, /* UART2 */
> > > > + [ASPEED_CLK_GATE_UART1CLK] ={ 15, -1, "uart1clk-gate",
> > > "uart", CLK_IS_CRITICAL }, /* UART1 */
> > > > + [ASPEED_CLK_GATE_UART2CLK] ={ 16, -1, "uart2clk-gate",
> > > "uart", CLK_IS_CRITICAL }, /* UART2 */
> > > > [ASPEED_CLK_GATE_UART5CLK] ={ 17, -1,
> "uart5clk-gate",
> > > "uart", 0 }, /* UART5 */
> > > > - [ASPEED_CLK_GATE_ESPICLK] = { 19, -1, "espiclk-gate",
> > > NULL, 0 }, /* eSPI */
> > > > + [ASPEED_CLK_GATE_ESPICLK] = { 19, -1, "espiclk-gate",
> > > NULL, CLK_IS_CRITICAL }, /* eSPI */
> > >
> > > This is fine for systems that have eSPI. For systems that do not use
> > > eSPI, the clocks are not "required".
> > >
> > > I was sent a similar patch by Jae some time ago:
> > >
> > >
> > > https://lore.kernel.org/openbmc/697a184b-ef99-a46e-bf98-4d339b3aafd8
> > > @lin
> > > ux.intel.com/
> > >
> > > Better is to associ
RE: [PATCH 0/3] Enable USB host for AST2600
Hello Joel, Have you take time review this patches ? Ryan Chen Tel : 886-3-5751185 ext:8857 > -Original Message- > From: Ryan Chen > Sent: Wednesday, September 30, 2020 12:08 PM > To: Joel Stanley ; Andrew Jeffery ; > linux-arm-ker...@lists.infradead.org; linux-asp...@lists.ozlabs.org; > linux-kernel@vger.kernel.org; linux-...@vger.kernel.org; BMC-SW > > Cc: Ryan Chen > Subject: [PATCH 0/3] Enable USB host for AST2600 > > The patches enable UHCI driver in AST2600 and also enable USB host in > aspeed-ast2600-evb.dts. > > Ryan Chen (3): > configs: aspeed: enable UHCI driver in defconfig > usb: host: add uhci compatible support for ast2600-uhci > ARM: dts: add ehci uhci enable in evb dts > > arch/arm/boot/dts/aspeed-ast2600-evb.dts | 8 > arch/arm/configs/aspeed_g5_defconfig | 1 + > drivers/usb/host/uhci-platform.c | 3 ++- > 3 files changed, 11 insertions(+), 1 deletion(-) > > -- > 2.17.1
RE: [RESEND PATCH] ARM: dts: aspeed-g6: Fix gpio memory region
> -Original Message- > From: Andrew Jeffery > Sent: Thursday, October 1, 2020 8:32 AM > To: Billy Tsai ; Rob Herring ; > Joel Stanley ; devicet...@vger.kernel.org; > linux-arm-ker...@lists.infradead.org; linux-asp...@lists.ozlabs.org; > linux-kernel@vger.kernel.org; Ryan Chen > Cc: BMC-SW > Subject: Re: [RESEND PATCH] ARM: dts: aspeed-g6: Fix gpio memory region > > > > On Thu, 1 Oct 2020, at 09:42, Andrew Jeffery wrote: > > Hi Billy, > > > > On Wed, 30 Sep 2020, at 18:36, Billy Tsai wrote: > > > Signed-off-by: Billy Tsai > > > --- > > > arch/arm/boot/dts/aspeed-g6.dtsi | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/arch/arm/boot/dts/aspeed-g6.dtsi > > > b/arch/arm/boot/dts/aspeed-g6.dtsi > > > index 97ca743363d7..b9ec8b579f73 100644 > > > --- a/arch/arm/boot/dts/aspeed-g6.dtsi > > > +++ b/arch/arm/boot/dts/aspeed-g6.dtsi > > > @@ -357,7 +357,7 @@ > > > #gpio-cells = <2>; > > > gpio-controller; > > > compatible = "aspeed,ast2600-gpio"; > > > - reg = <0x1e78 0x800>; > > > + reg = <0x1e78 0x500>; > > > > We took the 0x800 value from the memory space layout table in the > > datasheet for the 2600. Should that be updated too? Or are you just > > limiting the region to the registers currently described rather than the > allocated address space? > > Ah, actually, I see what's going on. We really have this layout (taking some > liberties): > > 0x1e785000 - 0x1e785500: PGPIO 3.3V > 0x1e785500 - 0x1e785600: SGPM1 > 0x1e785600 - 0x1e785700: SGPM2 > 0x1e785700 - 0x1e785740: SPGS1 > 0x1e785740 - 0x1e785780: SPGS2 > 0x1e785800 - 0x1e786000: PGPIO 1.8V > > Ryan: Can you change the address space layout table to reflect this? That way > it > still functions as a quick - but accurate - reference. Yes will resend the patch for update the table. 0x1e78 ~ 0x1e780400 PGPIO 3.3V 0x1e780500 - 0x1e780600: SGPM1 0x1e780600 - 0x1e780700: SGPM2 0x1e780700 - 0x1e780740: SPGS1 0x1e780740 - 0x1e780780: SPGS2 0x1e780800 - 0x1e781000: PGPIO 1.8V
[PATCH 2/3] usb: host: add uhci compatible support for ast2600-uhci
Add support for AST2600 SOC UHCI driver.
Signed-off-by: Ryan Chen
---
drivers/usb/host/uhci-platform.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/host/uhci-platform.c b/drivers/usb/host/uhci-platform.c
index 70dbd95c3f06..fa40fe125c2a 100644
--- a/drivers/usb/host/uhci-platform.c
+++ b/drivers/usb/host/uhci-platform.c
@@ -113,7 +113,8 @@ static int uhci_hcd_platform_probe(struct platform_device
*pdev)
num_ports);
}
if (of_device_is_compatible(np, "aspeed,ast2400-uhci") ||
- of_device_is_compatible(np, "aspeed,ast2500-uhci")) {
+ of_device_is_compatible(np, "aspeed,ast2500-uhci") ||
+ of_device_is_compatible(np, "aspeed,ast2600-uhci")) {
uhci->is_aspeed = 1;
dev_info(>dev,
"Enabled Aspeed implementation workarounds\n");
--
2.17.1
[PATCH 0/3] Enable USB host for AST2600
The patches enable UHCI driver in AST2600 and also enable USB host in aspeed-ast2600-evb.dts. Ryan Chen (3): configs: aspeed: enable UHCI driver in defconfig usb: host: add uhci compatible support for ast2600-uhci ARM: dts: add ehci uhci enable in evb dts arch/arm/boot/dts/aspeed-ast2600-evb.dts | 8 arch/arm/configs/aspeed_g5_defconfig | 1 + drivers/usb/host/uhci-platform.c | 3 ++- 3 files changed, 11 insertions(+), 1 deletion(-) -- 2.17.1
[PATCH 1/3] configs: aspeed: enable UHCI driver in defconfig
Enable UHCI driver in aspeed_g5_defconfig. Signed-off-by: Ryan Chen --- arch/arm/configs/aspeed_g5_defconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm/configs/aspeed_g5_defconfig b/arch/arm/configs/aspeed_g5_defconfig index 2bacd8c90f4b..a57009d1a3b8 100644 --- a/arch/arm/configs/aspeed_g5_defconfig +++ b/arch/arm/configs/aspeed_g5_defconfig @@ -212,6 +212,7 @@ CONFIG_USB_DYNAMIC_MINORS=y CONFIG_USB_EHCI_HCD=y CONFIG_USB_EHCI_ROOT_HUB_TT=y CONFIG_USB_EHCI_HCD_PLATFORM=y +CONFIG_USB_UHCI_HCD=y CONFIG_USB_GADGET=y CONFIG_USB_ASPEED_VHUB=y CONFIG_USB_CONFIGFS=y -- 2.17.1
[PATCH 3/3] ARM: dts: add ehci uhci enable in evb dts
Add EHCI UHCI enable build in aspeed-ast2600-evb.dts
Signed-off-by: Ryan Chen
---
arch/arm/boot/dts/aspeed-ast2600-evb.dts | 8
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
index 89be13197780..2772796e215e 100644
--- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
+++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
@@ -237,3 +237,11 @@
{
status = "okay";
};
+
+ {
+ status = "okay";
+};
+
+ {
+ status = "okay";
+};
--
2.17.1
RE: [PATCH 1/1] clk: aspeed: modify some default clks are critical
> From: Joel Stanley
> Sent: Tuesday, September 29, 2020 4:04 PM
> To: Ryan Chen ; Jae Hyun Yoo
> ; Andrew Jeffery
> Cc: Michael Turquette ; Stephen Boyd
> ; linux-...@vger.kernel.org; Linux ARM
> ; linux-aspeed
> ; Linux Kernel Mailing List
> ; BMC-SW
> Subject: Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical
>
> On Mon, 28 Sep 2020 at 07:01, Ryan Chen
> wrote:
> >
> > In ASPEED SoC LCLK is LPC clock for all SuperIO device, UART1/UART2
> > are default for Host SuperIO UART device, eSPI clk for Host eSPI bus
> > access eSPI slave channel, those clks can't be disable should keep
> > default, otherwise will affect Host side access SuperIO and SPI slave
> > device.
> >
> > Signed-off-by: Ryan Chen
> > ---
> > drivers/clk/clk-aspeed.c | 8 drivers/clk/clk-ast2600.c | 8
> >
> > 2 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c index
> > 411ff5fb2c07..d348c4fd3f9f 100644
> > --- a/drivers/clk/clk-aspeed.c
> > +++ b/drivers/clk/clk-aspeed.c
> > @@ -54,15 +54,15 @@ static const struct aspeed_gate_data aspeed_gates[]
> = {
> > [ASPEED_CLK_GATE_DCLK] ={ 5, -1, "dclk-gate",
> NULL, CLK_IS_CRITICAL }, /* DAC */
> > [ASPEED_CLK_GATE_REFCLK] = { 6, -1, "refclk-gate",
> "clkin", CLK_IS_CRITICAL },
> > [ASPEED_CLK_GATE_USBPORT2CLK] = { 7, 3, "usb-port2-gate",
> NULL, 0 }, /* USB2.0 Host port 2 */
> > - [ASPEED_CLK_GATE_LCLK] ={ 8, 5, "lclk-gate",
> NULL, 0 }, /* LPC */
> > + [ASPEED_CLK_GATE_LCLK] ={ 8, 5, "lclk-gate",
> NULL, CLK_IS_CRITICAL }, /* LPC */
> > [ASPEED_CLK_GATE_USBUHCICLK] = { 9, 15, "usb-uhci-gate",
> NULL, 0 }, /* USB1.1 (requires port 2 enabled) */
> > [ASPEED_CLK_GATE_D1CLK] = { 10, 13, "d1clk-gate",
> NULL, 0 }, /* GFX CRT */
> > [ASPEED_CLK_GATE_YCLK] ={ 13, 4, "yclk-gate",
> NULL, 0 }, /* HAC */
> > [ASPEED_CLK_GATE_USBPORT1CLK] = { 14, 14, "usb-port1-gate",
> NULL, 0 }, /* USB2 hub/USB2 host port 1/USB1.1 dev */
> > - [ASPEED_CLK_GATE_UART1CLK] ={ 15, -1, "uart1clk-gate",
> "uart", 0 }, /* UART1 */
> > - [ASPEED_CLK_GATE_UART2CLK] ={ 16, -1, "uart2clk-gate",
> "uart", 0 }, /* UART2 */
> > + [ASPEED_CLK_GATE_UART1CLK] ={ 15, -1, "uart1clk-gate",
> "uart", CLK_IS_CRITICAL }, /* UART1 */
> > + [ASPEED_CLK_GATE_UART2CLK] ={ 16, -1, "uart2clk-gate",
> "uart", CLK_IS_CRITICAL }, /* UART2 */
> > [ASPEED_CLK_GATE_UART5CLK] ={ 17, -1, "uart5clk-gate",
> "uart", 0 }, /* UART5 */
> > - [ASPEED_CLK_GATE_ESPICLK] = { 19, -1, "espiclk-gate",
> NULL, 0 }, /* eSPI */
> > + [ASPEED_CLK_GATE_ESPICLK] = { 19, -1, "espiclk-gate",
> NULL, CLK_IS_CRITICAL }, /* eSPI */
>
> This is fine for systems that have eSPI. For systems that do not use eSPI, the
> clocks are not "required".
>
> I was sent a similar patch by Jae some time ago:
>
>
> https://lore.kernel.org/openbmc/697a184b-ef99-a46e-bf98-4d339b3aafd8@lin
> ux.intel.com/
>
> Better is to associate drivers with these clocks, and those drivers will
> ensure
> they are left enabled.
>
> Alternatively, we will need to come up with a device tree binding to describe
> the hardware requirement that these clocks are left on.
>
ASPEED BMC SoC have SuperIO device that default enable, even without BMC fw
boot.
Host can use SUART1/SUART2/GPIO
That the reason even Linux kernel boot should not change the SoC default clk,
that is the impact.
Ryan
>
> > [ASPEED_CLK_GATE_MAC1CLK] = { 20, 11, "mac1clk-gate",
> "mac", 0 }, /* MAC1 */
> > [ASPEED_CLK_GATE_MAC2CLK] = { 21, 12, "mac2clk-gate",
> "mac", 0 }, /* MAC2 */
> > [ASPEED_CLK_GATE_RSACLK] = { 24, -1, "rsaclk-gate",
> NULL, 0 }, /* RSA */
> > diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c
> > index bbacaccad554..6802a2d5bbe2 100644
> > --- a/drivers/clk/clk-ast2600.c
> > +++ b/drivers/clk/clk-ast2600.c
> > @@ -86,8 +86,8 @@ static const struct aspeed_gate_data aspeed_g6_gates[]
> = {
> > /* Reserved 26 */
> > [ASPEED_CLK_GATE_EMMCCLK] = { 27, 16,
> "emmcclk-gate", NULL,0 }, /* For card clk */
>
RE: [PATCH 0/4] Remove LPC register partitioning
Hello Joel & Andrew,
Those patches are more organize for ASPEED SOC LPC register layout.
Does those patches have any feedback?
Ryan
> -Original Message-
> From: ChiaWei Wang
> Sent: Friday, September 11, 2020 4:21 PM
> To: Andrew Jeffery ; Joel Stanley
> Cc: Rob Herring ; Corey Minyard ;
> Linus Walleij ; Haiyue Wang
> ; Cyril Bur ; Robert
> Lippert ; Linux ARM
> ; linux-aspeed
> ; Linux Kernel Mailing List
> ; OpenBMC Maillist
> ; Ryan Chen
> Subject: RE: [PATCH 0/4] Remove LPC register partitioning
>
> Hello,
>
> Thanks for your prompt feedback.
>
> > -Original Message-
> > From: Andrew Jeffery
> > Sent: Friday, September 11, 2020 12:46 PM
> > To: Joel Stanley ; ChiaWei Wang
> >
> > Subject: Re: [PATCH 0/4] Remove LPC register partitioning
> >
> >
> > On Fri, 11 Sep 2020, at 13:33, Joel Stanley wrote:
> > > Hello,
> > >
> > > On Fri, 11 Sep 2020 at 03:46, Chia-Wei, Wang
> > > wrote:
> > > >
> > > > The LPC controller has no concept of the BMC and the Host partitions.
> > > > The incorrect partitioning can impose unnecessary range
> > > > restrictions on register access through the syscon regmap interface.
> > > >
> > > > For instance, HICRB contains the I/O port address configuration of
> > > > KCS channel 1/2. However, the KCS#1/#2 drivers cannot access HICRB
> > > > as it is located at the other LPC partition.
> >
> > Thanks for addressing this, I've regretted that choice for a while now.
> >
> > The split was rooted in trying to support pinmux while not being
> > across every detail of the LPC controller, and so I made some poor
> > decisions.
> >
> > > >
> > > > In addition, to be backward compatible, the newly added HW control
> > > > bits could be added at any reserved bits over the LPC addressing space.
> > > >
> > > > Thereby, this patch series aims to remove the LPC partitioning for
> > > > better driver development and maintenance.
> > >
> > > I support this cleanup. The only consideration is to be careful with
> > > breaking the driver/device-tree relationship. We either need to
> > > ensure the drivers remain compatible with both device trees.
> > >
> > > Another solution is to get agreement from all parties that for the
> > > LPC device the device tree is always the one shipped with the
> > > kernel, so it is okay to make incompatible changes.
> If it is possible, I would prefer this solution to avoid adding additional
> if-logic
> for the compatibility support in the driver implementation.
> As the patch can be less change made to register offset definitions and leave
> the core logic untouched.
> > >
> > > While we are doing a cleanup, Andrew suggested we remove the
> > > detailed description of LPC out of the device tree. We would have
> > > the one LPC node, and create a LPC driver that creates all of the
> > > sub devices (snoop, FW cycles, kcs, bt, vuart). Andrew, can you
> > > elaborate on this plan?
> >
> > I dug up the conversation I had with Rob over a year ago about being
> > unhappy with what I'd cooked up.
> >
> > https://lore.kernel.org/linux-arm-kernel/CAL_JsqJ+sFDG8eKbV3gvmqVHx+ot
> > W
> > bki4dy213apzxgfhbx...@mail.gmail.com/
> >
> > But I think you covered most of the idea there: We have the LPC driver
> > create the subdevices and that moves the details out of the devicetree.
> > However, I haven't thought about it more than that, and I think there
> > are still problems with that idea. For instance, how we manage
> > configuration of those devices, and how to enable only the devices a
> > given platform actually cares about (i.e. the problems that devicetree
> > solves
> for us).
> Another concern to make centralized LPC driver implementation more
> complicated is the relationship with eSPI driver.
> AST2500 binds the reset control of LPC and eSPI together. If eSPI is used for
> the
> Host communication, the behavior in current "lpc-ctrl" should be skipped but
> not for KCS, BT, Snoop, etc.
> And this will be much easier to achieve by devicetree if LPC sub devices are
> individually described.
> >
> > It may be that the only way to do that is with platform code, and
> > that's not really a direction we should be going either.
> >
[PATCH 0/1] Modify ASPEED SoC some default clks are critical
This patch is modify for ASPEED SoC some default clks can't disable need keep default clk on. Ryan Chen (1): clk: aspeed: modify some default clks are critical drivers/clk/clk-aspeed.c | 8 drivers/clk/clk-ast2600.c | 8 2 files changed, 8 insertions(+), 8 deletions(-) -- 2.17.1
[PATCH 1/1] clk: aspeed: modify some default clks are critical
In ASPEED SoC LCLK is LPC clock for all SuperIO device, UART1/UART2 are
default for Host SuperIO UART device, eSPI clk for Host eSPI bus access
eSPI slave channel, those clks can't be disable should keep default,
otherwise will affect Host side access SuperIO and SPI slave device.
Signed-off-by: Ryan Chen
---
drivers/clk/clk-aspeed.c | 8
drivers/clk/clk-ast2600.c | 8
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
index 411ff5fb2c07..d348c4fd3f9f 100644
--- a/drivers/clk/clk-aspeed.c
+++ b/drivers/clk/clk-aspeed.c
@@ -54,15 +54,15 @@ static const struct aspeed_gate_data aspeed_gates[] = {
[ASPEED_CLK_GATE_DCLK] ={ 5, -1, "dclk-gate", NULL,
CLK_IS_CRITICAL }, /* DAC */
[ASPEED_CLK_GATE_REFCLK] = { 6, -1, "refclk-gate",
"clkin", CLK_IS_CRITICAL },
[ASPEED_CLK_GATE_USBPORT2CLK] = { 7, 3, "usb-port2-gate", NULL,
0 }, /* USB2.0 Host port 2 */
- [ASPEED_CLK_GATE_LCLK] ={ 8, 5, "lclk-gate", NULL,
0 }, /* LPC */
+ [ASPEED_CLK_GATE_LCLK] ={ 8, 5, "lclk-gate", NULL,
CLK_IS_CRITICAL }, /* LPC */
[ASPEED_CLK_GATE_USBUHCICLK] = { 9, 15, "usb-uhci-gate", NULL,
0 }, /* USB1.1 (requires port 2 enabled) */
[ASPEED_CLK_GATE_D1CLK] = { 10, 13, "d1clk-gate", NULL,
0 }, /* GFX CRT */
[ASPEED_CLK_GATE_YCLK] ={ 13, 4, "yclk-gate", NULL,
0 }, /* HAC */
[ASPEED_CLK_GATE_USBPORT1CLK] = { 14, 14, "usb-port1-gate", NULL,
0 }, /* USB2 hub/USB2 host port 1/USB1.1 dev */
- [ASPEED_CLK_GATE_UART1CLK] ={ 15, -1, "uart1clk-gate", "uart",
0 }, /* UART1 */
- [ASPEED_CLK_GATE_UART2CLK] ={ 16, -1, "uart2clk-gate", "uart",
0 }, /* UART2 */
+ [ASPEED_CLK_GATE_UART1CLK] ={ 15, -1, "uart1clk-gate", "uart",
CLK_IS_CRITICAL }, /* UART1 */
+ [ASPEED_CLK_GATE_UART2CLK] ={ 16, -1, "uart2clk-gate", "uart",
CLK_IS_CRITICAL }, /* UART2 */
[ASPEED_CLK_GATE_UART5CLK] ={ 17, -1, "uart5clk-gate", "uart",
0 }, /* UART5 */
- [ASPEED_CLK_GATE_ESPICLK] = { 19, -1, "espiclk-gate", NULL,
0 }, /* eSPI */
+ [ASPEED_CLK_GATE_ESPICLK] = { 19, -1, "espiclk-gate", NULL,
CLK_IS_CRITICAL }, /* eSPI */
[ASPEED_CLK_GATE_MAC1CLK] = { 20, 11, "mac1clk-gate", "mac",
0 }, /* MAC1 */
[ASPEED_CLK_GATE_MAC2CLK] = { 21, 12, "mac2clk-gate", "mac",
0 }, /* MAC2 */
[ASPEED_CLK_GATE_RSACLK] = { 24, -1, "rsaclk-gate",NULL,
0 }, /* RSA */
diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c
index bbacaccad554..6802a2d5bbe2 100644
--- a/drivers/clk/clk-ast2600.c
+++ b/drivers/clk/clk-ast2600.c
@@ -86,8 +86,8 @@ static const struct aspeed_gate_data aspeed_g6_gates[] = {
/* Reserved 26 */
[ASPEED_CLK_GATE_EMMCCLK] = { 27, 16, "emmcclk-gate", NULL,
0 }, /* For card clk */
/* Reserved 28/29/30 */
- [ASPEED_CLK_GATE_LCLK] = { 32, 32, "lclk-gate",NULL,
0 }, /* LPC */
- [ASPEED_CLK_GATE_ESPICLK] = { 33, -1, "espiclk-gate", NULL,
0 }, /* eSPI */
+ [ASPEED_CLK_GATE_LCLK] = { 32, 32, "lclk-gate",NULL,
CLK_IS_CRITICAL }, /* LPC */
+ [ASPEED_CLK_GATE_ESPICLK] = { 33, -1, "espiclk-gate", NULL,
CLK_IS_CRITICAL }, /* eSPI */
[ASPEED_CLK_GATE_REF1CLK] = { 34, -1, "ref1clk-gate",
"clkin", CLK_IS_CRITICAL },
/* Reserved 35 */
[ASPEED_CLK_GATE_SDCLK] = { 36, 56, "sdclk-gate", NULL,
0 }, /* SDIO/SD */
@@ -102,8 +102,8 @@ static const struct aspeed_gate_data aspeed_g6_gates[] = {
[ASPEED_CLK_GATE_I3C5CLK] = { 45, 45, "i3c5clk-gate",NULL,
0 }, /* I3C5 */
[ASPEED_CLK_GATE_I3C6CLK] = { 46, 46, "i3c6clk-gate",NULL,
0 }, /* I3C6 */
[ASPEED_CLK_GATE_I3C7CLK] = { 47, 47, "i3c7clk-gate",NULL,
0 }, /* I3C7 */
- [ASPEED_CLK_GATE_UART1CLK] = { 48, -1, "uart1clk-gate", "uart",
0 }, /* UART1 */
- [ASPEED_CLK_GATE_UART2CLK] = { 49, -1, "uart2clk-gate", "uart",
0 }, /* UART2 */
+ [ASPEED_CLK_GATE_UART1CLK] = { 48, -1, "uart1clk-gate", "uart",
CLK_IS_CRITICAL }, /* UART1 */
+ [ASPEED_CLK_GATE_UART2CLK] = { 49, -1, "uart2clk-gate", "uart",
CLK_IS_CRITICAL }, /* UART2 */
[ASPEED_CLK_GATE_UART3CLK] = { 50, -1, "uart3clk-gate", "uart",
0 }, /* UART3 */
[ASPEED_CLK_GATE_UART4CLK] = { 51, -1, "uart4clk-gate", "uart",
0 }, /* UART4 */
[ASPEED_CLK_GATE_MAC3CLK] = { 52, 52, "mac3clk-gate",
"mac34", 0 }, /* MAC3 */
--
2.17.1
[PATCH v3] Staging: nvec: Removes repeated word typo in comment
Fix a comment typo. Signed-off-by: Ryan Kosta --- V3: Fix commit name drivers/staging/nvec/nvec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c index 360ec040774..a80996b2f5c 100644 --- a/drivers/staging/nvec/nvec.c +++ b/drivers/staging/nvec/nvec.c @@ -289,7 +289,7 @@ EXPORT_SYMBOL(nvec_write_async); * interrupt handlers. * * Returns: 0 on success, a negative error code on failure. - * The response message is returned in @msg. Shall be freed with + * The response message is returned in @msg. Shall be freed * with nvec_msg_free() once no longer used. * */ -- 2.20.1
[PATCH v2] Staging: nvec: nvec: fix double next comment
Changes since v1: * Made commit message more clear * Added description Note: previous patch named "[PATCH] fix double next comment in drivers/staging/nvec/nvec.c" >8--8< Fixes a comment typo. Signed-off-by: Ryan Kosta --- drivers/staging/nvec/nvec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c index 360ec040774..a80996b2f5c 100644 --- a/drivers/staging/nvec/nvec.c +++ b/drivers/staging/nvec/nvec.c @@ -289,7 +289,7 @@ EXPORT_SYMBOL(nvec_write_async); * interrupt handlers. * * Returns: 0 on success, a negative error code on failure. - * The response message is returned in @msg. Shall be freed with + * The response message is returned in @msg. Shall be freed * with nvec_msg_free() once no longer used. * */ -- 2.20.1
[PATCH] fix double next comment in drivers/staging/nvec/nvec.c
Signed-off-by: Ryan Kosta --- drivers/staging/nvec/nvec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c index 360ec040774..a80996b2f5c 100644 --- a/drivers/staging/nvec/nvec.c +++ b/drivers/staging/nvec/nvec.c @@ -289,7 +289,7 @@ EXPORT_SYMBOL(nvec_write_async); * interrupt handlers. * * Returns: 0 on success, a negative error code on failure. - * The response message is returned in @msg. Shall be freed with + * The response message is returned in @msg. Shall be freed * with nvec_msg_free() once no longer used. * */ -- 2.20.1
RE: [PATCH 2/4] clk: ast2600: Add functionality to the APLL clock
Hello Eddie,
For FSI design is request for two clock source.
one for FSI core, one for FSI bus clock.
In AST2600 FSI core clk is apll, FSI bus clock need apll/4.
So, you should fix for fsi clock input. Not set from fsi-master-driver.
And fsi-master driver div from clock source apll/4. Should not set for
apll from fsi-master driver.
> -Original Message-
> From: Joel Stanley
> Sent: Friday, September 11, 2020 9:30 AM
> To: Eddie James ; Ryan Chen
> ; Billy Tsai
> Cc: linux-...@vger.kernel.org; Linux Kernel Mailing List
> ; devicetree ;
> linux-aspeed ; linux-...@lists.ozlabs.org;
> Andrew Jeffery ; Rob Herring ;
> Stephen Boyd ; Michael Turquette
> ; Alistair Popple ; Jeremy
> Kerr
> Subject: Re: [PATCH 2/4] clk: ast2600: Add functionality to the APLL clock
>
> Ryan,
>
> This change adds support for setting the A-PLL in the 2600 so we can control
> the FSI frequency. Can your team please review it? If it is okay please reply
> with your Reviewed-by.
>
>
> On Thu, 10 Sep 2020 at 15:18, Eddie James wrote:
> >
> > Register a clock with it's own operations to describe the APLL on the
> > AST2600. The clock is controlled by an SCU register containing a
> > multiplier and divider of the 25MHz input clock.
> > The functionality to change the APLL is necessary to finely control
> > the FSI bus frequency.
> >
> > Signed-off-by: Eddie James
> > ---
> > drivers/clk/clk-ast2600.c | 177
> > +++---
> > 1 file changed, 165 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c
> > index 177368cac6dd..a147dffbaccc 100644
> > --- a/drivers/clk/clk-ast2600.c
> > +++ b/drivers/clk/clk-ast2600.c
> > @@ -4,6 +4,7 @@
> >
> > #define pr_fmt(fmt) "clk-ast2600: " fmt
> >
> > +#include
> > #include
> > #include
> > #include
> > @@ -15,7 +16,7 @@
> >
> > #include "clk-aspeed.h"
> >
> > -#define ASPEED_G6_NUM_CLKS 71
> > +#define ASPEED_G6_NUM_CLKS 72
> >
> > #define ASPEED_G6_SILICON_REV 0x004
> >
> > @@ -31,6 +32,7 @@
> > #define ASPEED_G6_CLK_SELECTION1 0x300
> > #define ASPEED_G6_CLK_SELECTION2 0x304
> > #define ASPEED_G6_CLK_SELECTION4 0x310
> > +#define ASPEED_G6_CLK_SELECTION5 0x314
> >
> > #define ASPEED_HPLL_PARAM 0x200
> > #define ASPEED_APLL_PARAM 0x210
> > @@ -116,7 +118,7 @@ static const struct aspeed_gate_data
> aspeed_g6_gates[] = {
> > [ASPEED_CLK_GATE_UART11CLK] = { 59, -1,
> "uart11clk-gate", "uartx", 0 }, /* UART11 */
> > [ASPEED_CLK_GATE_UART12CLK] = { 60, -1,
> "uart12clk-gate", "uartx", 0 }, /* UART12 */
> > [ASPEED_CLK_GATE_UART13CLK] = { 61, -1,
> "uart13clk-gate", "uartx", 0 }, /* UART13 */
> > - [ASPEED_CLK_GATE_FSICLK]= { 62, 59, "fsiclk-gate",
> NULL,0 }, /* FSI */
> > + [ASPEED_CLK_GATE_FSICLK]= { 62, 59, "fsiclk-gate",
> "aplln", CLK_SET_RATE_PARENT }, /* FSI */
>
> Why do we call this apll*n* ?
>
> I believe the apll is also the parent of the sdclk.
>
> Designs that use FSI do not use the sdclk, but it should be added to the table
> for completeness.
>
> > };
> >
> > static const struct clk_div_table ast2600_eclk_div_table[] = { @@
> > -187,24 +189,166 @@ static struct clk_hw *ast2600_calc_pll(const char
> *name, u32 val)
> > mult, div);
> > };
> >
> > -static struct clk_hw *ast2600_calc_apll(const char *name, u32 val)
> > +/*
> > + * APLL Frequency: F = 25MHz * (2 - od) * [(m + 2) / (n + 1)] */
> > +static void ast2600_apll_get_params(unsigned int *div, unsigned int
> > +*mul)
> > {
> > - unsigned int mult, div;
> > + u32 val = readl(scu_g6_base + ASPEED_APLL_PARAM);
> >
> > if (val & BIT(20)) {
> > /* Pass through mode */
> > - mult = div = 1;
> > + *mul = *div = 1;
> > } else {
> > - /* F = 25Mhz * (2-od) * [(m + 2) / (n + 1)] */
> > u32 m = (val >> 5) & 0x3f;
> > u32 od = (val >> 4) & 0x1;
> > u32 n = val & 0xf;
> >
> > - mult = (2 - od) * (m + 2);
> > - div = n + 1;
> > +
RE: [PATCH 3/5] i2c: aspeed: Mask IRQ status to relevant bits
> -Original Message- > From: Tao Ren [mailto:rentao.b...@gmail.com] > Sent: Wednesday, August 26, 2020 4:05 AM > To: Eddie James > Cc: Joel Stanley ; devicetree ; > linux-aspeed ; dmitry.torok...@gmail.com; > Brendan Higgins ; Linux Kernel Mailing List > ; Rob Herring ; > linux-...@vger.kernel.org; linux-in...@vger.kernel.org; Ryan Chen > > Subject: Re: [PATCH 3/5] i2c: aspeed: Mask IRQ status to relevant bits > > On Tue, Aug 25, 2020 at 02:47:51PM -0500, Eddie James wrote: > > > > On 8/25/20 1:38 AM, Joel Stanley wrote: > > > On Thu, 20 Aug 2020 at 16:12, Eddie James > wrote: > > > > Mask the IRQ status to only the bits that the driver checks. This > > > > prevents excessive driver warnings when operating in slave mode > > > > when additional bits are set that the driver doesn't handle. > > > > > > > > Signed-off-by: Eddie James > > > > --- > > > > drivers/i2c/busses/i2c-aspeed.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/drivers/i2c/busses/i2c-aspeed.c > > > > b/drivers/i2c/busses/i2c-aspeed.c index 31268074c422..abf40f2af8b4 > > > > 100644 > > > > --- a/drivers/i2c/busses/i2c-aspeed.c > > > > +++ b/drivers/i2c/busses/i2c-aspeed.c > > > > @@ -604,6 +604,7 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, > void *dev_id) > > > > writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE, > > > > bus->base + ASPEED_I2C_INTR_STS_REG); > > > > readl(bus->base + ASPEED_I2C_INTR_STS_REG); > > > > + irq_received &= 0xf000; > > > > irq_remaining = irq_received; > > > This would defeat the check for irq_remaining. I don't think we want to do > this. > > > > > > Can you explain why these bits are being set in slave mode? > > > > > > No, I don't have any documentation for the bits that are masked off > > here, so I don't know why they would get set. > > > > The check for irq_remaining is still useful for detecting that the > > driver state machine might be out of sync with what the master is doing. > > I have a similar patch in my local tree, and the reason being: AST2600 I2C > Controller may set I2CD10[25:24] to report Current Slave Parking Status > (defined in new register I2CS24) even though the new register mode is off. The > 2 bits can be ignored in legacy mode, and Ryan from ASPEED could confirm it. Yes, in AST2600 i2cd10[25:24] will be the same with new mode register i2cs24[25:24] Thanks Tao. > > > Cheers, > > Tao
[PATCH v2] Bug fix to ELF Loader which rejects valid ELFs
Check was incorrectly being applied to size of elf phdrs, instead of the number. The ELF standard allows for up to 65535 headers, but the check was being compared to the number of headers multiplied by the size of a program header. --- fs/binfmt_elf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 13d053982dd7..f21896f250d2 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -445,11 +445,11 @@ static struct elf_phdr *load_elf_phdrs(const struct elfhdr *elf_ex, goto out; /* Sanity check the number of program headers... */ - /* ...and their total size. */ - size = sizeof(struct elf_phdr) * elf_ex->e_phnum; - if (size == 0 || size > 65536 || size > ELF_MIN_ALIGN) + if (elf_ex->e_phnum == 0) goto out; +size = array_size(sizeof(*elf_phdata), elf_ex->e_phnum); + elf_phdata = kmalloc(size, GFP_KERNEL); if (!elf_phdata) goto out; -- 2.17.1 smime.p7s Description: S/MIME cryptographic signature
RE: [PATCH] Bug fix to ELF Loader which rejects valid ELFs
> -Original Message- > From: Matthew Wilcox > Sent: Tuesday, August 11, 2020 11:05 AM > To: Burrow, Ryan - 0553 - MITLL > Cc: linux-fsde...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] Bug fix to ELF Loader which rejects valid ELFs > > On Tue, Aug 11, 2020 at 02:44:08PM +, Burrow, Ryan - 0553 - MITLL wrote: > > /* Sanity check the number of program headers... */ > > - /* ...and their total size. */ > > - size = sizeof(struct elf_phdr) * elf_ex->e_phnum; > > - if (size == 0 || size > 65536 || size > ELF_MIN_ALIGN) > > + if (elf_ex->e_phnum == 0 || elf_ex->phnum > 65535) > > umm, did you compile-test this? > My apologies, I had made these edits in the context of other changes which I didn't want to include in this patch - I replicated these changes individually and (mistakenly) assumed I had done so correctly. > assuming you meant e_phnum, it's a 16-bit quantity, so it can't be bigger > than 65535. > > > goto out; > > > > + size = sizeof(struct elf_phdr) * elf_ex->e_phnum; > > use array_size() here? > That's a good catch - I had really just moved the previous allocation of size down to after the check, but I'll use array_size instead. Just to confirm the correct process, should I do an inline response with the updated commit, or submit a new patch email? smime.p7s Description: S/MIME cryptographic signature
[PATCH] Bug fix to ELF Loader which rejects valid ELFs
Check was incorrectly being applied to size of elf phdrs, instead of the number. The ELF standard allows for up to 65535 headers, but the check was being compared to the number of headers multiplied by the size of a program header. --- fs/binfmt_elf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 9fe3b51c116a..5605b5205f84 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -445,11 +445,11 @@ static struct elf_phdr *load_elf_phdrs(const struct elfhdr *elf_ex, goto out; /* Sanity check the number of program headers... */ - /* ...and their total size. */ - size = sizeof(struct elf_phdr) * elf_ex->e_phnum; - if (size == 0 || size > 65536 || size > ELF_MIN_ALIGN) + if (elf_ex->e_phnum == 0 || elf_ex->phnum > 65535) goto out; + size = sizeof(struct elf_phdr) * elf_ex->e_phnum; + elf_phdata = kmalloc(size, GFP_KERNEL); if (!elf_phdata) goto out; -- 2.17.1 smime.p7s Description: S/MIME cryptographic signature
RE: [PATCH v0 linux master] i2c/busses: Avoid i2c interrupt status clear race condition.
> In AST2600 there have a slow peripheral bus between CPU and i2c > controller. > Therefore GIC i2c interrupt status clear have delay timing, when CPU > issue write clear i2c controller interrupt status. > To avoid this issue, the driver need have read after write clear at > i2c ISR. > > Signed-off-by: ryan_chen >Applied to for-current with a Fixes tag, thanks! Please, try to add one next >time and please also check how the subsystem formats the $subject line. [Ryan Chen] Thanks your review, will add fixes tag at subject.
RE: [PATCH v0 linux master] i2c/busses: Avoid i2c interrupt status clear race condition.
> > And is there maybe a Fixes: tag for it? > > [Ryan Chen] Yes it is a fix patch. > > I meant this (from submitting-patches.rst): >It fixes the original implementation of the driver basically. It's just a >classic posted-write fix. The write to clear the pending interrupt is >asynchronous, so you can get spurrious ones if you return from the handler >before it has percolated to the HW. >I assume it's just more visible on the 2600 because of the cores are >significantly faster but the IO bus is still as dumb. >Ryan: You could always add a Fixed-by: tag that specifies the commit that >added the initial driver... [Ryan Chen] Thanks Ben.
RE: [PATCH v0 linux master] i2c/busses: Avoid i2c interrupt status clear race condition.
-Original Message-
From: Wolfram Sang [mailto:w...@the-dreams.de]
Sent: Wednesday, April 29, 2020 3:54 PM
To: Ryan Chen
Cc: Brendan Higgins ; Benjamin Herrenschmidt
; Joel Stanley ; Andrew Jeffery
; linux-...@vger.kernel.org; open...@lists.ozlabs.org;
linux-arm-ker...@lists.infradead.org; linux-asp...@lists.ozlabs.org;
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v0 linux master] i2c/busses: Avoid i2c interrupt status
clear race condition.
On Wed, Apr 29, 2020 at 11:37:37AM +0800, ryan_chen wrote:
> In AST2600 there have a slow peripheral bus between CPU and i2c
> controller.
> Therefore GIC i2c interrupt status clear have delay timing, when CPU
> issue write clear i2c controller interrupt status.
> To avoid this issue, the driver need have read after write clear at
> i2c ISR.
>
> Signed-off-by: ryan_chen
v0? is it a prototype?
[Ryan Chen] It is not prototype it is official patch.
And is there maybe a Fixes: tag for it?
[Ryan Chen] Yes it is a fix patch.
> ---
> drivers/i2c/busses/i2c-aspeed.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c
> b/drivers/i2c/busses/i2c-aspeed.c index 07c1993274c5..f51702d86a90
> 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -603,6 +603,7 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void
> *dev_id)
> /* Ack all interrupts except for Rx done */
> writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
> bus->base + ASPEED_I2C_INTR_STS_REG);
> + readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> irq_remaining = irq_received;
>
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> @@ -645,9 +646,11 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void
> *dev_id)
> irq_received, irq_handled);
>
> /* Ack Rx done */
> - if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
> + if (irq_received & ASPEED_I2CD_INTR_RX_DONE) {
> writel(ASPEED_I2CD_INTR_RX_DONE,
> bus->base + ASPEED_I2C_INTR_STS_REG);
> + readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> + }
> spin_unlock(>lock);
> return irq_remaining ? IRQ_NONE : IRQ_HANDLED; }
> --
> 2.17.1
>
[PATCH] net: bcmgenet: use ethtool_op_get_ts_info()
This change enables the use of SW timestamping on the Raspberry Pi 4.
bcmgenet's transmit function bcmgenet_xmit() implements software
timestamping. However the SOF_TIMESTAMPING_TX_SOFTWARE capability was
missing and only SOF_TIMESTAMPING_RX_SOFTWARE was announced. By using
ethtool_ops bcmgenet_ethtool_ops() as get_ts_info(), the
SOF_TIMESTAMPING_TX_SOFTWARE capability is announced.
Similar to commit a8f5cb9e7991 ("smsc95xx: use ethtool_op_get_ts_info()")
Signed-off-by: Ryan M. Collins
---
drivers/net/ethernet/broadcom/genet/bcmgenet.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 1586316eb6f1..12cb77ef1081 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1124,6 +1124,7 @@ static const struct ethtool_ops bcmgenet_ethtool_ops = {
.set_coalesce = bcmgenet_set_coalesce,
.get_link_ksettings = bcmgenet_get_link_ksettings,
.set_link_ksettings = bcmgenet_set_link_ksettings,
+ .get_ts_info= ethtool_op_get_ts_info,
};
/* Power down the unimac, based on mode. */
--
2.23.0
Re: [PATCH 2/2] usb: pci-quirks: Minor cleanup for AMD PLL quirk
On Fri, Jul 5, 2019 at 3:10 PM Alan Stern wrote:
>
> On Thu, 4 Jul 2019, Ryan Kennedy wrote:
>
> > usb_amd_find_chipset_info() is used for chipset detection for
> > several quirks. It is strange that its return value indicates
> > the need for the PLL quirk, which means it is often ignored.
> > This patch adds a function specifically for checking the PLL
> > quirk like the other ones. Additionally, rename probe_result to
> > something more appropriate.
> >
> > Signed-off-by: Ryan Kennedy
>
> > @@ -322,6 +317,13 @@ bool usb_amd_prefetch_quirk(void)
> > }
> > EXPORT_SYMBOL_GPL(usb_amd_prefetch_quirk);
> >
> > +bool usb_amd_quirk_pll_check(void)
> > +{
> > + usb_amd_find_chipset_info();
> > + return amd_chipset.need_pll_quirk;
> > +}
> > +EXPORT_SYMBOL_GPL(usb_amd_quirk_pll_check);
>
> I really don't see the point of separating out all but one line into a
> different function. You might as well just rename
> usb_amd_find_chipset_info to usb_amd_quirk_pll_check (along with the
> other code adjustments) and be done with it.
I did this for consistency with the others:
usb_amd_prefetch_quirk()
usb_amd_hang_symptom_quirk()
usb_hcd_amd_remote_wakeup_quirk()
They all need to ensure the chipset information exists then decide if
the particular quirk should be applied to the chipset.
Ryan
>
> However, in the end I don't care if you still want to do this. Either
> way:
>
> Acked-by: Alan Stern
>
> Alan Stern
>
Re: [PATCH 1/2] usb: pci-quirks: Correct AMD PLL quirk detection
On Fri, Jul 5, 2019 at 1:22 AM Greg KH wrote: > > On Thu, Jul 04, 2019 at 11:35:28AM -0400, Ryan Kennedy wrote: > > The AMD PLL USB quirk is incorrectly enabled on newer Ryzen > > chipsets. The logic in usb_amd_find_chipset_info currently checks > > for unaffected chipsets rather than affected ones. This broke > > once a new chipset was added in e788787ef. It makes more sense > > to reverse the logic so it won't need to be updated as new > > chipsets are added. Note that the core of the workaround in > > usb_amd_quirk_pll does correctly check the chipset. > > > > Signed-off-by: Ryan Kennedy > > --- > > drivers/usb/host/pci-quirks.c | 31 +++ > > 1 file changed, 19 insertions(+), 12 deletions(-) > > Should this be backported to stable kernels? The bug is fairly harmless, so I wouldn't say it's a must-have. I only noticed this because I saw the log message and was curious what the quirk was for. The fix saves us calling usb_amd_quirk_pll() and taking the lock in there. Others here should know better than I what's stable worthy. Ryan > > thanks, > > greg k-h
[PATCH 2/2] usb: pci-quirks: Minor cleanup for AMD PLL quirk
usb_amd_find_chipset_info() is used for chipset detection for
several quirks. It is strange that its return value indicates
the need for the PLL quirk, which means it is often ignored.
This patch adds a function specifically for checking the PLL
quirk like the other ones. Additionally, rename probe_result to
something more appropriate.
Signed-off-by: Ryan Kennedy
---
drivers/usb/host/ehci-pci.c | 4 ++--
drivers/usb/host/ohci-pci.c | 2 +-
drivers/usb/host/pci-quirks.c | 30 --
drivers/usb/host/pci-quirks.h | 2 +-
drivers/usb/host/xhci-pci.c | 2 +-
5 files changed, 21 insertions(+), 19 deletions(-)
diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index fe9422d3bcdc..b0882c13a1d1 100644
--- a/drivers/usb/host/ehci-pci.c
+++ b/drivers/usb/host/ehci-pci.c
@@ -149,7 +149,7 @@ static int ehci_pci_setup(struct usb_hcd *hcd)
break;
case PCI_VENDOR_ID_AMD:
/* AMD PLL quirk */
- if (usb_amd_find_chipset_info())
+ if (usb_amd_quirk_pll_check())
ehci->amd_pll_fix = 1;
/* AMD8111 EHCI doesn't work, according to AMD errata */
if (pdev->device == 0x7463) {
@@ -186,7 +186,7 @@ static int ehci_pci_setup(struct usb_hcd *hcd)
break;
case PCI_VENDOR_ID_ATI:
/* AMD PLL quirk */
- if (usb_amd_find_chipset_info())
+ if (usb_amd_quirk_pll_check())
ehci->amd_pll_fix = 1;
/*
diff --git a/drivers/usb/host/ohci-pci.c b/drivers/usb/host/ohci-pci.c
index fbcd34911025..208abaaef8f6 100644
--- a/drivers/usb/host/ohci-pci.c
+++ b/drivers/usb/host/ohci-pci.c
@@ -152,7 +152,7 @@ static int ohci_quirk_amd700(struct usb_hcd *hcd)
{
struct ohci_hcd *ohci = hcd_to_ohci(hcd);
- if (usb_amd_find_chipset_info())
+ if (usb_amd_quirk_pll_check())
ohci->flags |= OHCI_QUIRK_AMD_PLL;
/* SB800 needs pre-fetch fix */
diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index ad05c27b3a7b..f6d04491df60 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -132,7 +132,7 @@ static struct amd_chipset_info {
struct amd_chipset_type sb_type;
int isoc_reqs;
int probe_count;
- int probe_result;
+ bool need_pll_quirk;
} amd_chipset;
static DEFINE_SPINLOCK(amd_lock);
@@ -201,11 +201,11 @@ void sb800_prefetch(struct device *dev, int on)
}
EXPORT_SYMBOL_GPL(sb800_prefetch);
-int usb_amd_find_chipset_info(void)
+static void usb_amd_find_chipset_info(void)
{
unsigned long flags;
struct amd_chipset_info info;
- int need_pll_quirk = 0;
+ info.need_pll_quirk = 0;
spin_lock_irqsave(_lock, flags);
@@ -213,7 +213,7 @@ int usb_amd_find_chipset_info(void)
if (amd_chipset.probe_count > 0) {
amd_chipset.probe_count++;
spin_unlock_irqrestore(_lock, flags);
- return amd_chipset.probe_result;
+ return;
}
memset(, 0, sizeof(info));
spin_unlock_irqrestore(_lock, flags);
@@ -224,19 +224,19 @@ int usb_amd_find_chipset_info(void)
switch (info.sb_type.gen) {
case AMD_CHIPSET_SB700:
- need_pll_quirk = info.sb_type.rev <= 0x3B;
+ info.need_pll_quirk = info.sb_type.rev <= 0x3B;
break;
case AMD_CHIPSET_SB800:
case AMD_CHIPSET_HUDSON2:
case AMD_CHIPSET_BOLTON:
- need_pll_quirk = 1;
+ info.need_pll_quirk = 1;
break;
default:
- need_pll_quirk = 0;
+ info.need_pll_quirk = 0;
break;
}
- if (!need_pll_quirk) {
+ if (!info.need_pll_quirk) {
if (info.smbus_dev) {
pci_dev_put(info.smbus_dev);
info.smbus_dev = NULL;
@@ -259,7 +259,6 @@ int usb_amd_find_chipset_info(void)
}
}
- need_pll_quirk = info.probe_result = 1;
printk(KERN_DEBUG "QUIRK: Enable AMD PLL fix\n");
commit:
@@ -270,7 +269,6 @@ int usb_amd_find_chipset_info(void)
/* Mark that we where here */
amd_chipset.probe_count++;
- need_pll_quirk = amd_chipset.probe_result;
spin_unlock_irqrestore(_lock, flags);
@@ -283,10 +281,7 @@ int usb_amd_find_chipset_info(void)
amd_chipset = info;
spin_unlock_irqrestore(_lock, flags);
}
-
- return need_pll_quirk;
}
-EXPORT_SYMBOL_GPL(usb_amd_find_chipset_info);
int usb_hcd_amd_remote_wakeup_quirk(struct pci_dev *pdev)
{
@@ -322,6 +317,13 @@ bool usb_amd_prefetch_quirk(void)
}
EXPORT_SYMBOL_GPL(usb_amd_prefetch_quirk);
+bool usb_amd_quirk_pll_check(void)
+{
+ usb_amd_find
[PATCH 1/2] usb: pci-quirks: Correct AMD PLL quirk detection
The AMD PLL USB quirk is incorrectly enabled on newer Ryzen
chipsets. The logic in usb_amd_find_chipset_info currently checks
for unaffected chipsets rather than affected ones. This broke
once a new chipset was added in e788787ef. It makes more sense
to reverse the logic so it won't need to be updated as new
chipsets are added. Note that the core of the workaround in
usb_amd_quirk_pll does correctly check the chipset.
Signed-off-by: Ryan Kennedy
---
drivers/usb/host/pci-quirks.c | 31 +++
1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 3ce71cbfbb58..ad05c27b3a7b 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -205,7 +205,7 @@ int usb_amd_find_chipset_info(void)
{
unsigned long flags;
struct amd_chipset_info info;
- int ret;
+ int need_pll_quirk = 0;
spin_lock_irqsave(_lock, flags);
@@ -219,21 +219,28 @@ int usb_amd_find_chipset_info(void)
spin_unlock_irqrestore(_lock, flags);
if (!amd_chipset_sb_type_init()) {
- ret = 0;
goto commit;
}
- /* Below chipset generations needn't enable AMD PLL quirk */
- if (info.sb_type.gen == AMD_CHIPSET_UNKNOWN ||
- info.sb_type.gen == AMD_CHIPSET_SB600 ||
- info.sb_type.gen == AMD_CHIPSET_YANGTZE ||
- (info.sb_type.gen == AMD_CHIPSET_SB700 &&
- info.sb_type.rev > 0x3b)) {
+ switch (info.sb_type.gen) {
+ case AMD_CHIPSET_SB700:
+ need_pll_quirk = info.sb_type.rev <= 0x3B;
+ break;
+ case AMD_CHIPSET_SB800:
+ case AMD_CHIPSET_HUDSON2:
+ case AMD_CHIPSET_BOLTON:
+ need_pll_quirk = 1;
+ break;
+ default:
+ need_pll_quirk = 0;
+ break;
+ }
+
+ if (!need_pll_quirk) {
if (info.smbus_dev) {
pci_dev_put(info.smbus_dev);
info.smbus_dev = NULL;
}
- ret = 0;
goto commit;
}
@@ -252,7 +259,7 @@ int usb_amd_find_chipset_info(void)
}
}
- ret = info.probe_result = 1;
+ need_pll_quirk = info.probe_result = 1;
printk(KERN_DEBUG "QUIRK: Enable AMD PLL fix\n");
commit:
@@ -263,7 +270,7 @@ int usb_amd_find_chipset_info(void)
/* Mark that we where here */
amd_chipset.probe_count++;
- ret = amd_chipset.probe_result;
+ need_pll_quirk = amd_chipset.probe_result;
spin_unlock_irqrestore(_lock, flags);
@@ -277,7 +284,7 @@ int usb_amd_find_chipset_info(void)
spin_unlock_irqrestore(_lock, flags);
}
- return ret;
+ return need_pll_quirk;
}
EXPORT_SYMBOL_GPL(usb_amd_find_chipset_info);
--
2.21.0
[PATCH 0/2] usb: pci-quirks: AMD PLL quirk fix
This series contains a minor fix for the AMD PLL USB quirk plus some clean up to the related code. Ryan Kennedy (2): usb: pci-quirks: Correct AMD PLL quirk detection usb: pci-quirks: Minor cleanup for AMD PLL quirk drivers/usb/host/ehci-pci.c | 4 ++-- drivers/usb/host/ohci-pci.c | 2 +- drivers/usb/host/pci-quirks.c | 45 +-- drivers/usb/host/pci-quirks.h | 2 +- drivers/usb/host/xhci-pci.c | 2 +- 5 files changed, 32 insertions(+), 23 deletions(-) -- 2.21.0
RE: [PATCH 1/2] i2c: aspeed: allow to customize base clock divisor
Hello Tao, Let me more clear. When you set (3, 15, 14) the device sometimes response nack. but when you set (4, 7, 7), the device always ack. Am I right? Ryan -Original Message- From: Tao Ren [mailto:tao...@fb.com] Sent: Thursday, June 20, 2019 3:57 PM To: Ryan Chen ; Brendan Higgins Cc: Mark Rutland ; devicetree ; linux-asp...@lists.ozlabs.org; OpenBMC Maillist ; Linux Kernel Mailing List ; Rob Herring ; Linux ARM ; linux-...@vger.kernel.org Subject: Re: [PATCH 1/2] i2c: aspeed: allow to customize base clock divisor On 6/20/19 12:29 AM, Ryan Chen wrote: > Hello Tao, > Our recommend about clk divider setting is follow the datasheet clock > setting table for clock divisor. > > Ryan Thanks Ryan for the response. Could you also share some recommendations/hints on how to solve the intermittent i2c transaction failures on Facebook AST2500 BMC platforms? BTW, the patch is not aimed at modifying the existing formula of calculating clock settings in i2c-aspeed driver: people still get the recommended settings by default. The goal of the patch is to allow people to customize clock settings in case the default/recommended one doesn't work. Cheers, Tao
RE: [PATCH 1/2] i2c: aspeed: allow to customize base clock divisor
Hello Tao, Our recommend about clk divider setting is follow the datasheet clock setting table for clock divisor. Ryan -Original Message- From: Linux-aspeed [mailto:linux-aspeed-bounces+ryan_chen=aspeedtech@lists.ozlabs.org] On Behalf Of Tao Ren Sent: Thursday, June 20, 2019 6:33 AM To: Brendan Higgins Cc: Mark Rutland ; devicetree ; linux-asp...@lists.ozlabs.org; OpenBMC Maillist ; Linux Kernel Mailing List ; Rob Herring ; Linux ARM ; linux-...@vger.kernel.org Subject: Re: [PATCH 1/2] i2c: aspeed: allow to customize base clock divisor On 6/19/19 2:25 PM, Brendan Higgins wrote: > On Wed, Jun 19, 2019 at 2:00 PM Tao Ren wrote: >> >> Some intermittent I2C transaction failures are observed on Facebook >> CMM and Minipack (ast2500) BMC platforms, because slave devices (such >> as CPLD, BIC and etc.) NACK the address byte sometimes. The issue can >> be resolved by increasing base clock divisor which affects ASPEED I2C >> Controller's base clock and other AC timing parameters. >> >> This patch allows to customize ASPEED I2C Controller's base clock >> divisor in device tree. > > First off, are you sure you actually need this? > > You should be able to achieve an effectively equivalent result by just > lowering the `bus-frequency` property specified in the DT. The > `bus-frequency` property ultimately determines all the register > values, and you should be able to set it to whatever you want by > refering to the Aspeed documentation. > > Nevertheless, the code that determines the correct dividers from the > frequency is based on the tables in the Aspeed documentation. I don't > think the equation makes sense when the base_clk_divisor is fixed; I > mean it will probably just set the other divisor to max or min > depending on the values chosen. I think if someone really wants to > program this parameter manually, they probably want to set the other > parameters manually too. Thank you for the quick response, Brendan. Aspeed I2C bus frequency is defined by 3 parameters (base_clk_divisor, clk_high_width, clk_low_width), and I choose base_clk_divisor because it controls all the Aspeed I2C timings (such as setup time and hold time). Once base_clk_divisor is decided (either by the current logic in i2c-aspeed driver or manually set in device tree), clk_high_width and clk_low_width will be calculated by i2c-aspeed driver to meet the specified I2C bus speed. For example, by setting I2C bus frequency to 100KHz on AST2500 platform, (base_clock_divisor, clk_high_width, clk_low_width) is set to (3, 15, 14) by our driver. But some slave devices (on CMM i2c-8 and Minipack i2c-0) NACK byte transactions with the default timing setting: the issue can be resolved by setting base_clk_divisor to 4, and (clk_high_width, clk_low_width) will be set to (7, 7) by our i2c-aspeed driver to achieve similar I2C bus speed. Not sure if my answer helps to address your concerns, but kindly let me know if you have further questions/suggestions. Thanks, Tao
Re: [RFC V2 06/11] soc: mediatek: add MT8183 dvfsrc support
Hi Henry,
On Tue, Apr 30, 2019 at 2:45 AM Henry Chen wrote:
>
> Add dvfsrc driver for MT8183
>
> Signed-off-by: Henry Chen
> ---
> drivers/soc/mediatek/Kconfig | 15 ++
> drivers/soc/mediatek/Makefile | 1 +
> drivers/soc/mediatek/mtk-dvfsrc.c | 347
> ++
> include/soc/mediatek/mtk_dvfsrc.h | 22 +++
> 4 files changed, 385 insertions(+)
> create mode 100644 drivers/soc/mediatek/mtk-dvfsrc.c
> create mode 100644 include/soc/mediatek/mtk_dvfsrc.h
>
> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> index 17bd759..2721fd6 100644
> --- a/drivers/soc/mediatek/Kconfig
> +++ b/drivers/soc/mediatek/Kconfig
> @@ -24,6 +24,21 @@ config MTK_INFRACFG
> INFRACFG controller contains various infrastructure registers not
> directly associated to any device.
>
> +config MTK_DVFSRC
> + bool "MediaTek DVFSRC Support"
> + depends on ARCH_MEDIATEK
> + default ARCH_MEDIATEK
> + select MTK_INFRACFG
> + select PM_GENERIC_DOMAINS if PM
> + depends on MTK_SCPSYS
> + help
> + Say yes here to add support for the MediaTek DVFSRC (dynamic voltage
> + and frequency scaling resource collector) found
> + on different MediaTek SoCs. The DVFSRC is a proprietary
> + hardware which is used to collect all the requests from
> + system and turn into the decision of minimum Vcore voltage
> + and minimum DRAM frequency to fulfill those requests.
> +
> config MTK_PMIC_WRAP
> tristate "MediaTek PMIC Wrapper Support"
> depends on RESET_CONTROLLER
> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> index b9dbad6..cd9d63f 100644
> --- a/drivers/soc/mediatek/Makefile
> +++ b/drivers/soc/mediatek/Makefile
> @@ -1,4 +1,5 @@
> obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
> +obj-$(CONFIG_MTK_DVFSRC) += mtk-dvfsrc.o
> obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o mtk-scpsys-ext.o
> obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
> obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> diff --git a/drivers/soc/mediatek/mtk-dvfsrc.c
> b/drivers/soc/mediatek/mtk-dvfsrc.c
> new file mode 100644
> index 000..e54a654
> --- /dev/null
> +++ b/drivers/soc/mediatek/mtk-dvfsrc.c
> @@ -0,0 +1,347 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018 MediaTek Inc.
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "mtk-scpsys.h"
> +
> +#define DVFSRC_IDLE0x00
> +#define DVFSRC_GET_TARGET_LEVEL(x) (((x) >> 0) & 0x)
> +#define DVFSRC_GET_CURRENT_LEVEL(x)(((x) >> 16) & 0x)
> +
> +#define MT8183_DVFSRC_OPP_LP4 0
> +#define MT8183_DVFSRC_OPP_LP4X 1
> +#define MT8183_DVFSRC_OPP_LP3 2
> +
> +struct dvfsrc_opp {
> + u32 vcore_opp;
> + u32 dram_opp;
> +};
> +
> +struct dvfsrc_domain {
> + u32 id;
> + u32 state;
> +};
> +
> +struct mtk_dvfsrc;
> +struct dvfsrc_soc_data {
> + const int *regs;
> + u32 num_opp;
> + u32 num_domains;
> + const struct dvfsrc_opp **opps;
> + struct dvfsrc_domain *domains;
> + int (*get_target_level)(struct mtk_dvfsrc *dvfsrc);
> + int (*get_current_level)(struct mtk_dvfsrc *dvfsrc);
> + void (*set_dram_bw)(struct mtk_dvfsrc *dvfsrc, u64 bw);
> + void (*set_opp_level)(struct mtk_dvfsrc *dvfsrc, u32 level);
> +};
> +
> +struct mtk_dvfsrc {
> + struct device *dev;
> + struct clk *clk_dvfsrc;
> + const struct dvfsrc_soc_data *dvd;
> + int dram_type;
> + void __iomem *regs;
> + struct mutex lock;
> + struct notifier_block scpsys_notifier;
> +};
> +
> +static u32 dvfsrc_read(struct mtk_dvfsrc *dvfs, u32 offset)
> +{
> + return readl(dvfs->regs + dvfs->dvd->regs[offset]);
> +}
> +
> +static void dvfsrc_write(struct mtk_dvfsrc *dvfs, u32 offset, u32 val)
> +{
> + writel(val, dvfs->regs + dvfs->dvd->regs[offset]);
> +}
> +
> +enum dvfsrc_regs {
> + DVFSRC_SW_REQ,
> + DVFSRC_LEVEL,
> + DVFSRC_SW_BW_0,
> + DVFSRC_LAST,
> +};
> +
> +static const int mt8183_regs[] = {
> + [DVFSRC_SW_REQ] = 0x4,
> + [DVFSRC_LEVEL] =0xDC,
> + [DVFSRC_SW_BW_0] = 0x160,
> + [DVFSRC_LAST] = 0x308,
> +};
> +
> +static bool dvfsrc_is_idle(struct mtk_dvfsrc *dvfsrc)
> +{
> + if (!dvfsrc->dvd->get_target_level)
> + return true;
> +
> + return dvfsrc->dvd->get_target_level(dvfsrc) == DVFSRC_IDLE;
> +}
> +
> +static int dvfsrc_wait_for_idle(struct mtk_dvfsrc *dvfsrc)
> +{
> + unsigned long timeout;
> +
> + timeout = jiffies + usecs_to_jiffies(1000);
> +
> + do {
> + if (dvfsrc_is_idle(dvfsrc))
> + return 0;
> + } while (!time_after(jiffies, timeout));
This all seems like it would be better handled by
RE: [PATCH 2/2] arm64: allwinner: a64: Add Oceanic A64-5inMFD initial support
Jagan, The compatible is for the real hardware name as Maxime previously explained. The platform is described in the latter part. Please use: compatible = "oceanic,5205-5inmfd", "allwinner,sun50i-a64"; Thanks, Ryan Pannell Development Team Lead Oceanic Systems (UK) Ltd -Original Message- From: Jagan Teki Sent: 06 March 2019 09:36 To: Maxime Ripard Cc: Ryan Pannell ; Chen-Yu Tsai ; Rob Herring ; Mark Rutland ; Michael Trimarchi ; linux-arm-kernel ; devicetree ; linux-kernel ; linux-amar...@amarulasolutions.com Subject: Re: [PATCH 2/2] arm64: allwinner: a64: Add Oceanic A64-5inMFD initial support On Wed, Mar 6, 2019 at 2:57 PM Maxime Ripard wrote: > > On Tue, Mar 05, 2019 at 04:35:04PM +, Ryan Pannell wrote: > > We don't mind if it has to be: > > compatible = "oceanic,5inmfd", "allwinner,sun50i-a64"; > > > > However, to avoid confusion, might it be worth including the part number > > (We list this as '5205 5" MFD') so this would become: > > compatible = "oceanic,5205-5inmfd", "allwinner,sun50i-a64"; > > > > Perhaps this solves the issue. Please let me know thoughts. > > Yep, that works for me A64 instead of part-number 5205 seems less confused isn't it? or we need to give more priority with real hardware definition?
RE: [PATCH 2/2] arm64: allwinner: a64: Add Oceanic A64-5inMFD initial support
We don't mind if it has to be:
compatible = "oceanic,5inmfd", "allwinner,sun50i-a64";
However, to avoid confusion, might it be worth including the part number (We
list this as '5205 5" MFD') so this would become:
compatible = "oceanic,5205-5inmfd", "allwinner,sun50i-a64";
Perhaps this solves the issue. Please let me know thoughts.
Ryan Pannell
Development Team Lead
Oceanic Systems (UK) Ltd
-Original Message-
From: Jagan Teki
Sent: 05 March 2019 16:08
To: Maxime Ripard
Cc: Chen-Yu Tsai ; Rob Herring ; Mark
Rutland ; Ryan Pannell ; Michael
Trimarchi ; linux-arm-kernel
; devicetree
; linux-kernel ;
linux-amar...@amarulasolutions.com
Subject: Re: [PATCH 2/2] arm64: allwinner: a64: Add Oceanic A64-5inMFD initial
support
On Tue, Mar 5, 2019 at 9:29 PM Maxime Ripard wrote:
>
> On Fri, Mar 01, 2019 at 10:27:14PM +0530, Jagan Teki wrote:
> > On Fri, Mar 1, 2019 at 8:27 PM Maxime Ripard
> > wrote:
> > >
> > > On Wed, Feb 27, 2019 at 09:49:23PM +0530, Jagan Teki wrote:
> > > > On Wed, Feb 27, 2019 at 9:03 PM Maxime Ripard
> > > > wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Tue, Feb 26, 2019 at 11:32:40AM +0530, Jagan Teki wrote:
> > > > > > Oceanic A64-5inMFD is a 5 inch Multi function display
> > > > > > baseboard designed to mount SoPine SOM.
> > > > > >
> > > > > > Key features:
> > > > > > - Allwinner A64 Cortex-A53
> > > > > > - Mali-400MP2 GPU
> > > > > > - AXP803 PMIC
> > > > > > - 2GB DDR3 RAM
> > > > > > - SD Slot
> > > > > > - SPI-NOR flash
> > > > > > - EMAC, RTL8211E
> > > > > > - MCP2515 CAN
> > > > > > - MIPI-DSI
> > > > > > - Goodix 911 CTP
> > > > > > - USB Host
> > > > > > - 12V DC power supply
> > > > > >
> > > > > > Signed-off-by: Jagan Teki
> > > > > > ---
> > > > > > arch/arm64/boot/dts/allwinner/Makefile| 1 +
> > > > > > .../allwinner/sun50i-a64-oceanic-5inmfd.dts | 46
> > > > > > +++
> > > > > > 2 files changed, 47 insertions(+) create mode 100644
> > > > > > arch/arm64/boot/dts/allwinner/sun50i-a64-oceanic-5inmfd.dts
> > > > > >
> > > > > > diff --git a/arch/arm64/boot/dts/allwinner/Makefile
> > > > > > b/arch/arm64/boot/dts/allwinner/Makefile
> > > > > > index a5fb1eaa8acf..ec39fe856117 100644
> > > > > > --- a/arch/arm64/boot/dts/allwinner/Makefile
> > > > > > +++ b/arch/arm64/boot/dts/allwinner/Makefile
> > > > > > @@ -2,6 +2,7 @@
> > > > > > dtb-$(CONFIG_ARCH_SUNXI) += sun50i-a64-amarula-relic.dtb
> > > > > > dtb-$(CONFIG_ARCH_SUNXI) += sun50i-a64-bananapi-m64.dtb
> > > > > > sun50i-a64-bananapi-m64-icn6211.dtb
> > > > > > dtb-$(CONFIG_ARCH_SUNXI) += sun50i-a64-nanopi-a64.dtb
> > > > > > +dtb-$(CONFIG_ARCH_SUNXI) += sun50i-a64-oceanic-5inmfd.dtb
> > > > > > dtb-$(CONFIG_ARCH_SUNXI) += sun50i-a64-olinuxino.dtb
> > > > > > dtb-$(CONFIG_ARCH_SUNXI) += sun50i-a64-orangepi-win.dtb
> > > > > > dtb-$(CONFIG_ARCH_SUNXI) += sun50i-a64-pine64-lts.dtb diff
> > > > > > --git
> > > > > > a/arch/arm64/boot/dts/allwinner/sun50i-a64-oceanic-5inmfd.dt
> > > > > > s
> > > > > > b/arch/arm64/boot/dts/allwinner/sun50i-a64-oceanic-5inmfd.dt
> > > > > > s
> > > > > > new file mode 100644
> > > > > > index ..d73d1f55acb9
> > > > > > --- /dev/null
> > > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-oceanic-5inmf
> > > > > > +++ d.dts
> > > > > > @@ -0,0 +1,46 @@
> > > > > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > > > > +/*
> > > > > > + * Copyright (C) 2019 Oceanic Systems (UK) Ltd.
> > > > > > + * Copyright (C) 2019 Amarula Solutions B.V.
> > > > > > + * Author: Jagan Teki */
> > > > > > +
> > > > > > +/dts-v1/;
> > > > > > +
> > > > > > +#include "sun50i-a64-sopine.dtsi"
> > > > > > +
> > > > > > +/ {
> > > > > > + model =
[PATCH v2 4/4] tty: serial: qcom_geni_serial: Use u32 for register variables
Use u32 rather than unsigned long for register variables for clarity and
consistency.
Signed-off-by: Ryan Case
Reviewed-by: Stephen Boyd
Reviewed-by: Evan Green
---
Changes in v2:
- Updated commit message
- Updated missed rxstale variable
drivers/tty/serial/qcom_geni_serial.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c
b/drivers/tty/serial/qcom_geni_serial.c
index 626bf7d399aa..6d8a44c12596 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -760,12 +760,12 @@ static void qcom_geni_serial_handle_tx(struct uart_port
*uport, bool done,
static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
{
- unsigned int m_irq_status;
- unsigned int s_irq_status;
- unsigned int geni_status;
+ u32 m_irq_en;
+ u32 m_irq_status;
+ u32 s_irq_status;
+ u32 geni_status;
struct uart_port *uport = dev;
unsigned long flags;
- unsigned int m_irq_en;
bool drop_rx = false;
struct tty_port *tport = >state->port;
struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
@@ -844,7 +844,7 @@ static void qcom_geni_serial_shutdown(struct uart_port
*uport)
static int qcom_geni_serial_port_setup(struct uart_port *uport)
{
struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
- unsigned int rxstale = DEFAULT_BITS_PER_CHAR * STALE_TIMEOUT;
+ u32 rxstale = DEFAULT_BITS_PER_CHAR * STALE_TIMEOUT;
u32 proto;
if (uart_console(uport)) {
@@ -943,14 +943,14 @@ static void qcom_geni_serial_set_termios(struct uart_port
*uport,
struct ktermios *termios, struct ktermios *old)
{
unsigned int baud;
- unsigned int bits_per_char;
- unsigned int tx_trans_cfg;
- unsigned int tx_parity_cfg;
- unsigned int rx_trans_cfg;
- unsigned int rx_parity_cfg;
- unsigned int stop_bit_len;
+ u32 bits_per_char;
+ u32 tx_trans_cfg;
+ u32 tx_parity_cfg;
+ u32 rx_trans_cfg;
+ u32 rx_parity_cfg;
+ u32 stop_bit_len;
unsigned int clk_div;
- unsigned long ser_clk_cfg;
+ u32 ser_clk_cfg;
struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
unsigned long clk_rate;
--
2.20.1.97.g81188d93c3-goog
[PATCH v2 2/4] tty: serial: qcom_geni_serial: Remove set_rfr_wm() and related variables
The variables of tx_wm and rx_wm were set to the same define value in
all cases, never updated, and the define was sometimes used
interchangably. Remove the variables/function and use the fixed value.
Signed-off-by: Ryan Case
Reviewed-by: Evan Green
---
Changes in v2:
- Removed CONSOLE from UART_CONSOLE_RX_WM
drivers/tty/serial/qcom_geni_serial.c | 25 -
1 file changed, 4 insertions(+), 21 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c
b/drivers/tty/serial/qcom_geni_serial.c
index e2cb3f7d1a7c..22d3cb4b3b39 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -89,7 +89,7 @@
#define DEF_FIFO_DEPTH_WORDS 16
#define DEF_TX_WM 2
#define DEF_FIFO_WIDTH_BITS32
-#define UART_CONSOLE_RX_WM 2
+#define UART_RX_WM 2
#define MAX_LOOPBACK_CFG 3
#ifdef CONFIG_CONSOLE_POLL
@@ -105,9 +105,6 @@ struct qcom_geni_serial_port {
u32 tx_fifo_depth;
u32 tx_fifo_width;
u32 rx_fifo_depth;
- u32 tx_wm;
- u32 rx_wm;
- u32 rx_rfr;
enum geni_se_xfer_mode xfer_mode;
bool setup;
int (*handle_rx)(struct uart_port *uport, u32 bytes, bool drop);
@@ -365,9 +362,7 @@ static int qcom_geni_serial_get_char(struct uart_port
*uport)
static void qcom_geni_serial_poll_put_char(struct uart_port *uport,
unsigned char c)
{
- struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
-
- writel(port->tx_wm, uport->membase + SE_GENI_TX_WATERMARK_REG);
+ writel(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
qcom_geni_serial_setup_tx(uport, 1);
WARN_ON(!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
M_TX_FIFO_WATERMARK_EN, true));
@@ -576,7 +571,7 @@ static void qcom_geni_serial_start_tx(struct uart_port
*uport)
irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
- writel(port->tx_wm, uport->membase +
+ writel(DEF_TX_WM, uport->membase +
SE_GENI_TX_WATERMARK_REG);
writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
}
@@ -848,17 +843,6 @@ static void get_tx_fifo_size(struct qcom_geni_serial_port
*port)
(port->tx_fifo_depth * port->tx_fifo_width) / BITS_PER_BYTE;
}
-static void set_rfr_wm(struct qcom_geni_serial_port *port)
-{
- /*
-* Set RFR (Flow off) to FIFO_DEPTH - 2.
-* RX WM level at 10% RX_FIFO_DEPTH.
-* TX WM level at 10% TX_FIFO_DEPTH.
-*/
- port->rx_rfr = port->rx_fifo_depth - 2;
- port->rx_wm = UART_CONSOLE_RX_WM;
- port->tx_wm = DEF_TX_WM;
-}
static void qcom_geni_serial_shutdown(struct uart_port *uport)
{
@@ -899,7 +883,6 @@ static int qcom_geni_serial_port_setup(struct uart_port
*uport)
get_tx_fifo_size(port);
- set_rfr_wm(port);
writel(rxstale, uport->membase + SE_UART_RX_STALE_CNT);
/*
* Make an unconditional cancel on the main sequencer to reset
@@ -912,7 +895,7 @@ static int qcom_geni_serial_port_setup(struct uart_port
*uport)
false, true, false);
geni_se_config_packing(>se, BITS_PER_BYTE, port->rx_bytes_pw,
false, false, true);
- geni_se_init(>se, port->rx_wm, port->rx_rfr);
+ geni_se_init(>se, UART_RX_WM, port->rx_fifo_depth - 2);
geni_se_select_mode(>se, port->xfer_mode);
if (!uart_console(uport)) {
port->rx_fifo = devm_kcalloc(uport->dev,
--
2.20.1.97.g81188d93c3-goog
[PATCH v2 1/4] tty: serial: qcom_geni_serial: Remove use of *_relaxed() and mb()
A frequent side comment has been to remove the use of writel_relaxed,
readl_relaxed, and mb. This reduces driver complexity and the _relaxed
variants were not known to provide any noticeable performance benefit.
Signed-off-by: Ryan Case
Reviewed-by: Evan Green
---
Changes in v2:
- Expanded commit message
- Coalesced lines where possible
drivers/tty/serial/qcom_geni_serial.c | 191 +++---
1 file changed, 80 insertions(+), 111 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c
b/drivers/tty/serial/qcom_geni_serial.c
index 2b1e73193dad..e2cb3f7d1a7c 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -230,7 +230,7 @@ static unsigned int qcom_geni_serial_get_mctrl(struct
uart_port *uport)
if (uart_console(uport) || !uart_cts_enabled(uport)) {
mctrl |= TIOCM_CTS;
} else {
- geni_ios = readl_relaxed(uport->membase + SE_GENI_IOS);
+ geni_ios = readl(uport->membase + SE_GENI_IOS);
if (!(geni_ios & IO2_DATA_IN))
mctrl |= TIOCM_CTS;
}
@@ -248,7 +248,7 @@ static void qcom_geni_serial_set_mctrl(struct uart_port
*uport,
if (!(mctrl & TIOCM_RTS))
uart_manual_rfr = UART_MANUAL_RFR_EN | UART_RFR_NOT_READY;
- writel_relaxed(uart_manual_rfr, uport->membase + SE_UART_MANUAL_RFR);
+ writel(uart_manual_rfr, uport->membase + SE_UART_MANUAL_RFR);
}
static const char *qcom_geni_serial_get_type(struct uart_port *uport)
@@ -277,9 +277,6 @@ static bool qcom_geni_serial_poll_bit(struct uart_port
*uport,
unsigned int fifo_bits;
unsigned long timeout_us = 2;
- /* Ensure polling is not re-ordered before the prior writes/reads */
- mb();
-
if (uport->private_data) {
port = to_dev_port(uport, uport);
baud = port->baud;
@@ -299,7 +296,7 @@ static bool qcom_geni_serial_poll_bit(struct uart_port
*uport,
*/
timeout_us = DIV_ROUND_UP(timeout_us, 10) * 10;
while (timeout_us) {
- reg = readl_relaxed(uport->membase + offset);
+ reg = readl(uport->membase + offset);
if ((bool)(reg & field) == set)
return true;
udelay(10);
@@ -312,7 +309,7 @@ static void qcom_geni_serial_setup_tx(struct uart_port
*uport, u32 xmit_size)
{
u32 m_cmd;
- writel_relaxed(xmit_size, uport->membase + SE_UART_TX_TRANS_LEN);
+ writel(xmit_size, uport->membase + SE_UART_TX_TRANS_LEN);
m_cmd = UART_START_TX << M_OPCODE_SHFT;
writel(m_cmd, uport->membase + SE_GENI_M_CMD0);
}
@@ -325,13 +322,13 @@ static void qcom_geni_serial_poll_tx_done(struct
uart_port *uport)
done = qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
M_CMD_DONE_EN, true);
if (!done) {
- writel_relaxed(M_GENI_CMD_ABORT, uport->membase +
+ writel(M_GENI_CMD_ABORT, uport->membase +
SE_GENI_M_CMD_CTRL_REG);
irq_clear |= M_CMD_ABORT_EN;
qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
M_CMD_ABORT_EN, true);
}
- writel_relaxed(irq_clear, uport->membase + SE_GENI_M_IRQ_CLEAR);
+ writel(irq_clear, uport->membase + SE_GENI_M_IRQ_CLEAR);
}
static void qcom_geni_serial_abort_rx(struct uart_port *uport)
@@ -341,8 +338,8 @@ static void qcom_geni_serial_abort_rx(struct uart_port
*uport)
writel(S_GENI_CMD_ABORT, uport->membase + SE_GENI_S_CMD_CTRL_REG);
qcom_geni_serial_poll_bit(uport, SE_GENI_S_CMD_CTRL_REG,
S_GENI_CMD_ABORT, false);
- writel_relaxed(irq_clear, uport->membase + SE_GENI_S_IRQ_CLEAR);
- writel_relaxed(FORCE_DEFAULT, uport->membase + GENI_FORCE_DEFAULT_REG);
+ writel(irq_clear, uport->membase + SE_GENI_S_IRQ_CLEAR);
+ writel(FORCE_DEFAULT, uport->membase + GENI_FORCE_DEFAULT_REG);
}
#ifdef CONFIG_CONSOLE_POLL
@@ -351,19 +348,13 @@ static int qcom_geni_serial_get_char(struct uart_port
*uport)
u32 rx_fifo;
u32 status;
- status = readl_relaxed(uport->membase + SE_GENI_M_IRQ_STATUS);
- writel_relaxed(status, uport->membase + SE_GENI_M_IRQ_CLEAR);
-
- status = readl_relaxed(uport->membase + SE_GENI_S_IRQ_STATUS);
- writel_relaxed(status, uport->membase + SE_GENI_S_IRQ_CLEAR);
+ status = readl(uport->membase + SE_GENI_M_IRQ_STATUS);
+ writel(status, uport->membase + SE_GENI_M_IRQ_CLEAR);
- /*
-* Ensure the writes to clear interrupts is not re-ordered after
-* reading the data.
-*/
- mb();
+ status = readl(upo
[PATCH v2 3/4] tty: serial: qcom_geni_serial: Remove xfer_mode variable
The driver only supports FIFO mode so setting and checking this variable
is unnecessary. If DMA support is ever added then such checks can be
introduced.
Signed-off-by: Ryan Case
Reviewed-by: Evan Green
---
Changes in v2:
- Fixed commit message typo
- Coalesced lines where possible
drivers/tty/serial/qcom_geni_serial.c | 67 ++-
1 file changed, 24 insertions(+), 43 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c
b/drivers/tty/serial/qcom_geni_serial.c
index 22d3cb4b3b39..626bf7d399aa 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -105,7 +105,6 @@ struct qcom_geni_serial_port {
u32 tx_fifo_depth;
u32 tx_fifo_width;
u32 rx_fifo_depth;
- enum geni_se_xfer_mode xfer_mode;
bool setup;
int (*handle_rx)(struct uart_port *uport, u32 bytes, bool drop);
unsigned int baud;
@@ -552,29 +551,20 @@ static int handle_rx_uart(struct uart_port *uport, u32
bytes, bool drop)
static void qcom_geni_serial_start_tx(struct uart_port *uport)
{
u32 irq_en;
- struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
u32 status;
- if (port->xfer_mode == GENI_SE_FIFO) {
- /*
-* readl ensures reading & writing of IRQ_EN register
-* is not re-ordered before checking the status of the
-* Serial Engine.
-*/
- status = readl(uport->membase + SE_GENI_STATUS);
- if (status & M_GENI_CMD_ACTIVE)
- return;
+ status = readl(uport->membase + SE_GENI_STATUS);
+ if (status & M_GENI_CMD_ACTIVE)
+ return;
- if (!qcom_geni_serial_tx_empty(uport))
- return;
+ if (!qcom_geni_serial_tx_empty(uport))
+ return;
- irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
- irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
+ irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
+ irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
- writel(DEF_TX_WM, uport->membase +
- SE_GENI_TX_WATERMARK_REG);
- writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
- }
+ writel(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
+ writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
}
static void qcom_geni_serial_stop_tx(struct uart_port *uport)
@@ -584,12 +574,8 @@ static void qcom_geni_serial_stop_tx(struct uart_port
*uport)
struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
- irq_en &= ~M_CMD_DONE_EN;
- if (port->xfer_mode == GENI_SE_FIFO) {
- irq_en &= ~M_TX_FIFO_WATERMARK_EN;
- writel(0, uport->membase +
-SE_GENI_TX_WATERMARK_REG);
- }
+ irq_en &= ~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN);
+ writel(0, uport->membase + SE_GENI_TX_WATERMARK_REG);
writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
status = readl(uport->membase + SE_GENI_STATUS);
/* Possible stop tx is called multiple times. */
@@ -619,15 +605,13 @@ static void qcom_geni_serial_start_rx(struct uart_port
*uport)
geni_se_setup_s_cmd(>se, UART_START_READ, 0);
- if (port->xfer_mode == GENI_SE_FIFO) {
- irq_en = readl(uport->membase + SE_GENI_S_IRQ_EN);
- irq_en |= S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN;
- writel(irq_en, uport->membase + SE_GENI_S_IRQ_EN);
+ irq_en = readl(uport->membase + SE_GENI_S_IRQ_EN);
+ irq_en |= S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN;
+ writel(irq_en, uport->membase + SE_GENI_S_IRQ_EN);
- irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
- irq_en |= M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN;
- writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
- }
+ irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
+ irq_en |= M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN;
+ writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
}
static void qcom_geni_serial_stop_rx(struct uart_port *uport)
@@ -637,15 +621,13 @@ static void qcom_geni_serial_stop_rx(struct uart_port
*uport)
struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
u32 irq_clear = S_CMD_DONE_EN;
- if (port->xfer_mode == GENI_SE_FIFO) {
- irq_en = readl(uport->membase + SE_GENI_S_IRQ_EN);
- irq_en &= ~(S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN);
- writel(irq_en, uport->membase + SE_GENI_S_IRQ_EN);
+ irq_en = readl(uport->membase + SE_GENI_S_IRQ_EN);
+
[PATCH v2 0/4] tty: serial: qcom_geni_serial: Assorted cleanups
This is a series of cleanups for issues raised in prior reviews that were unrelated to the patches at hand. Changes in v2: - Updated commit messages - Removed CONSOLE from UART_CONSOLE_RX_WM - Coalesced lines where possible - Updated missed rxstale variable Ryan Case (4): tty: serial: qcom_geni_serial: Remove use of *_relaxed() and mb() tty: serial: qcom_geni_serial: Remove set_rfr_wm() and related variables tty: serial: qcom_geni_serial: Remove xfer_mode variable tty: serial: qcom_geni_serial: Use u32 for register variables drivers/tty/serial/qcom_geni_serial.c | 279 ++ 1 file changed, 106 insertions(+), 173 deletions(-) -- 2.20.1.97.g81188d93c3-goog
[PATCH 4/4] tty: serial: qcom_geni_serial: Use u32 for register variables
Signed-off-by: Ryan Case
---
drivers/tty/serial/qcom_geni_serial.c | 22 +++---
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c
b/drivers/tty/serial/qcom_geni_serial.c
index 3103aa0adc86..fa67a2ced420 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -765,12 +765,12 @@ static void qcom_geni_serial_handle_tx(struct uart_port
*uport, bool done,
static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
{
- unsigned int m_irq_status;
- unsigned int s_irq_status;
- unsigned int geni_status;
+ u32 m_irq_en;
+ u32 m_irq_status;
+ u32 s_irq_status;
+ u32 geni_status;
struct uart_port *uport = dev;
unsigned long flags;
- unsigned int m_irq_en;
bool drop_rx = false;
struct tty_port *tport = >state->port;
struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
@@ -948,14 +948,14 @@ static void qcom_geni_serial_set_termios(struct uart_port
*uport,
struct ktermios *termios, struct ktermios *old)
{
unsigned int baud;
- unsigned int bits_per_char;
- unsigned int tx_trans_cfg;
- unsigned int tx_parity_cfg;
- unsigned int rx_trans_cfg;
- unsigned int rx_parity_cfg;
- unsigned int stop_bit_len;
+ u32 bits_per_char;
+ u32 tx_trans_cfg;
+ u32 tx_parity_cfg;
+ u32 rx_trans_cfg;
+ u32 rx_parity_cfg;
+ u32 stop_bit_len;
unsigned int clk_div;
- unsigned long ser_clk_cfg;
+ u32 ser_clk_cfg;
struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
unsigned long clk_rate;
--
2.20.1.415.g653613c723-goog
[PATCH 3/4] tty: serial: qcom_geni_serial: Remove xfer_mode variable
The driver only supports FIFO mode so setting and checking this variable
is unnecessary. If DMA support is ever addedd then such checks can be
introduced.
Signed-off-by: Ryan Case
---
drivers/tty/serial/qcom_geni_serial.c | 66 ++-
1 file changed, 24 insertions(+), 42 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c
b/drivers/tty/serial/qcom_geni_serial.c
index 5521ed4a0708..3103aa0adc86 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -105,7 +105,6 @@ struct qcom_geni_serial_port {
u32 tx_fifo_depth;
u32 tx_fifo_width;
u32 rx_fifo_depth;
- enum geni_se_xfer_mode xfer_mode;
bool setup;
int (*handle_rx)(struct uart_port *uport, u32 bytes, bool drop);
unsigned int baud;
@@ -555,29 +554,20 @@ static int handle_rx_uart(struct uart_port *uport, u32
bytes, bool drop)
static void qcom_geni_serial_start_tx(struct uart_port *uport)
{
u32 irq_en;
- struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
u32 status;
- if (port->xfer_mode == GENI_SE_FIFO) {
- /*
-* readl ensures reading & writing of IRQ_EN register
-* is not re-ordered before checking the status of the
-* Serial Engine.
-*/
- status = readl(uport->membase + SE_GENI_STATUS);
- if (status & M_GENI_CMD_ACTIVE)
- return;
+ status = readl(uport->membase + SE_GENI_STATUS);
+ if (status & M_GENI_CMD_ACTIVE)
+ return;
- if (!qcom_geni_serial_tx_empty(uport))
- return;
+ if (!qcom_geni_serial_tx_empty(uport))
+ return;
- irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
- irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
+ irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
+ irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
- writel(DEF_TX_WM, uport->membase +
- SE_GENI_TX_WATERMARK_REG);
- writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
- }
+ writel(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
+ writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
}
static void qcom_geni_serial_stop_tx(struct uart_port *uport)
@@ -588,11 +578,8 @@ static void qcom_geni_serial_stop_tx(struct uart_port
*uport)
irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
irq_en &= ~M_CMD_DONE_EN;
- if (port->xfer_mode == GENI_SE_FIFO) {
- irq_en &= ~M_TX_FIFO_WATERMARK_EN;
- writel(0, uport->membase +
-SE_GENI_TX_WATERMARK_REG);
- }
+ irq_en &= ~M_TX_FIFO_WATERMARK_EN;
+ writel(0, uport->membase + SE_GENI_TX_WATERMARK_REG);
writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
status = readl(uport->membase + SE_GENI_STATUS);
/* Possible stop tx is called multiple times. */
@@ -623,15 +610,13 @@ static void qcom_geni_serial_start_rx(struct uart_port
*uport)
geni_se_setup_s_cmd(>se, UART_START_READ, 0);
- if (port->xfer_mode == GENI_SE_FIFO) {
- irq_en = readl(uport->membase + SE_GENI_S_IRQ_EN);
- irq_en |= S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN;
- writel(irq_en, uport->membase + SE_GENI_S_IRQ_EN);
+ irq_en = readl(uport->membase + SE_GENI_S_IRQ_EN);
+ irq_en |= S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN;
+ writel(irq_en, uport->membase + SE_GENI_S_IRQ_EN);
- irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
- irq_en |= M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN;
- writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
- }
+ irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
+ irq_en |= M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN;
+ writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
}
static void qcom_geni_serial_stop_rx(struct uart_port *uport)
@@ -641,15 +626,13 @@ static void qcom_geni_serial_stop_rx(struct uart_port
*uport)
struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
u32 irq_clear = S_CMD_DONE_EN;
- if (port->xfer_mode == GENI_SE_FIFO) {
- irq_en = readl(uport->membase + SE_GENI_S_IRQ_EN);
- irq_en &= ~(S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN);
- writel(irq_en, uport->membase + SE_GENI_S_IRQ_EN);
+ irq_en = readl(uport->membase + SE_GENI_S_IRQ_EN);
+ irq_en &= ~(S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN);
+ writel(irq_en, uport->membase + SE_GENI_S_IRQ_EN);
- irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
-
[PATCH 2/4] tty: serial: qcom_geni_serial: Remove set_rfr_wm() and related variables
The variables of tx_wm and rx_wm were set to the same define value in
all cases, never updated, and the define was sometimes used
interchangably. Remove the variables/function and use the fixed value.
Signed-off-by: Ryan Case
---
drivers/tty/serial/qcom_geni_serial.c | 23 +++
1 file changed, 3 insertions(+), 20 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c
b/drivers/tty/serial/qcom_geni_serial.c
index dc95b96148ed..5521ed4a0708 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -105,9 +105,6 @@ struct qcom_geni_serial_port {
u32 tx_fifo_depth;
u32 tx_fifo_width;
u32 rx_fifo_depth;
- u32 tx_wm;
- u32 rx_wm;
- u32 rx_rfr;
enum geni_se_xfer_mode xfer_mode;
bool setup;
int (*handle_rx)(struct uart_port *uport, u32 bytes, bool drop);
@@ -365,9 +362,7 @@ static int qcom_geni_serial_get_char(struct uart_port
*uport)
static void qcom_geni_serial_poll_put_char(struct uart_port *uport,
unsigned char c)
{
- struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
-
- writel(port->tx_wm, uport->membase + SE_GENI_TX_WATERMARK_REG);
+ writel(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
qcom_geni_serial_setup_tx(uport, 1);
WARN_ON(!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
M_TX_FIFO_WATERMARK_EN, true));
@@ -579,7 +574,7 @@ static void qcom_geni_serial_start_tx(struct uart_port
*uport)
irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
- writel(port->tx_wm, uport->membase +
+ writel(DEF_TX_WM, uport->membase +
SE_GENI_TX_WATERMARK_REG);
writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
}
@@ -852,17 +847,6 @@ static void get_tx_fifo_size(struct qcom_geni_serial_port
*port)
(port->tx_fifo_depth * port->tx_fifo_width) / BITS_PER_BYTE;
}
-static void set_rfr_wm(struct qcom_geni_serial_port *port)
-{
- /*
-* Set RFR (Flow off) to FIFO_DEPTH - 2.
-* RX WM level at 10% RX_FIFO_DEPTH.
-* TX WM level at 10% TX_FIFO_DEPTH.
-*/
- port->rx_rfr = port->rx_fifo_depth - 2;
- port->rx_wm = UART_CONSOLE_RX_WM;
- port->tx_wm = DEF_TX_WM;
-}
static void qcom_geni_serial_shutdown(struct uart_port *uport)
{
@@ -903,7 +887,6 @@ static int qcom_geni_serial_port_setup(struct uart_port
*uport)
get_tx_fifo_size(port);
- set_rfr_wm(port);
writel(rxstale, uport->membase + SE_UART_RX_STALE_CNT);
/*
* Make an unconditional cancel on the main sequencer to reset
@@ -916,7 +899,7 @@ static int qcom_geni_serial_port_setup(struct uart_port
*uport)
false, true, false);
geni_se_config_packing(>se, BITS_PER_BYTE, port->rx_bytes_pw,
false, false, true);
- geni_se_init(>se, port->rx_wm, port->rx_rfr);
+ geni_se_init(>se, UART_CONSOLE_RX_WM, port->rx_fifo_depth - 2);
geni_se_select_mode(>se, port->xfer_mode);
if (!uart_console(uport)) {
port->rx_fifo = devm_kcalloc(uport->dev,
--
2.20.1.415.g653613c723-goog
[PATCH 0/4] tty: serial: qcom_geni_serial: Assorted cleanups
This is a series of cleanups for issues raised in prior reviews that were unrelated to the patches at hand. Ryan Case (4): tty: serial: qcom_geni_serial: Remove use of *_relaxed() and mb() tty: serial: qcom_geni_serial: Remove set_rfr_wm() and related variables tty: serial: qcom_geni_serial: Remove xfer_mode variable tty: serial: qcom_geni_serial: Use u32 for register variables drivers/tty/serial/qcom_geni_serial.c | 270 ++ 1 file changed, 104 insertions(+), 166 deletions(-) -- 2.20.1.415.g653613c723-goog
[PATCH 1/4] tty: serial: qcom_geni_serial: Remove use of *_relaxed() and mb()
A frequent side comment has been to remove the use of writel_relaxed,
readl_relaxed, and mb.
Signed-off-by: Ryan Case
---
drivers/tty/serial/qcom_geni_serial.c | 187 +++---
1 file changed, 80 insertions(+), 107 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c
b/drivers/tty/serial/qcom_geni_serial.c
index 2b1e73193dad..dc95b96148ed 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -230,7 +230,7 @@ static unsigned int qcom_geni_serial_get_mctrl(struct
uart_port *uport)
if (uart_console(uport) || !uart_cts_enabled(uport)) {
mctrl |= TIOCM_CTS;
} else {
- geni_ios = readl_relaxed(uport->membase + SE_GENI_IOS);
+ geni_ios = readl(uport->membase + SE_GENI_IOS);
if (!(geni_ios & IO2_DATA_IN))
mctrl |= TIOCM_CTS;
}
@@ -248,7 +248,7 @@ static void qcom_geni_serial_set_mctrl(struct uart_port
*uport,
if (!(mctrl & TIOCM_RTS))
uart_manual_rfr = UART_MANUAL_RFR_EN | UART_RFR_NOT_READY;
- writel_relaxed(uart_manual_rfr, uport->membase + SE_UART_MANUAL_RFR);
+ writel(uart_manual_rfr, uport->membase + SE_UART_MANUAL_RFR);
}
static const char *qcom_geni_serial_get_type(struct uart_port *uport)
@@ -277,9 +277,6 @@ static bool qcom_geni_serial_poll_bit(struct uart_port
*uport,
unsigned int fifo_bits;
unsigned long timeout_us = 2;
- /* Ensure polling is not re-ordered before the prior writes/reads */
- mb();
-
if (uport->private_data) {
port = to_dev_port(uport, uport);
baud = port->baud;
@@ -299,7 +296,7 @@ static bool qcom_geni_serial_poll_bit(struct uart_port
*uport,
*/
timeout_us = DIV_ROUND_UP(timeout_us, 10) * 10;
while (timeout_us) {
- reg = readl_relaxed(uport->membase + offset);
+ reg = readl(uport->membase + offset);
if ((bool)(reg & field) == set)
return true;
udelay(10);
@@ -312,7 +309,7 @@ static void qcom_geni_serial_setup_tx(struct uart_port
*uport, u32 xmit_size)
{
u32 m_cmd;
- writel_relaxed(xmit_size, uport->membase + SE_UART_TX_TRANS_LEN);
+ writel(xmit_size, uport->membase + SE_UART_TX_TRANS_LEN);
m_cmd = UART_START_TX << M_OPCODE_SHFT;
writel(m_cmd, uport->membase + SE_GENI_M_CMD0);
}
@@ -325,13 +322,13 @@ static void qcom_geni_serial_poll_tx_done(struct
uart_port *uport)
done = qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
M_CMD_DONE_EN, true);
if (!done) {
- writel_relaxed(M_GENI_CMD_ABORT, uport->membase +
+ writel(M_GENI_CMD_ABORT, uport->membase +
SE_GENI_M_CMD_CTRL_REG);
irq_clear |= M_CMD_ABORT_EN;
qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
M_CMD_ABORT_EN, true);
}
- writel_relaxed(irq_clear, uport->membase + SE_GENI_M_IRQ_CLEAR);
+ writel(irq_clear, uport->membase + SE_GENI_M_IRQ_CLEAR);
}
static void qcom_geni_serial_abort_rx(struct uart_port *uport)
@@ -341,8 +338,8 @@ static void qcom_geni_serial_abort_rx(struct uart_port
*uport)
writel(S_GENI_CMD_ABORT, uport->membase + SE_GENI_S_CMD_CTRL_REG);
qcom_geni_serial_poll_bit(uport, SE_GENI_S_CMD_CTRL_REG,
S_GENI_CMD_ABORT, false);
- writel_relaxed(irq_clear, uport->membase + SE_GENI_S_IRQ_CLEAR);
- writel_relaxed(FORCE_DEFAULT, uport->membase + GENI_FORCE_DEFAULT_REG);
+ writel(irq_clear, uport->membase + SE_GENI_S_IRQ_CLEAR);
+ writel(FORCE_DEFAULT, uport->membase + GENI_FORCE_DEFAULT_REG);
}
#ifdef CONFIG_CONSOLE_POLL
@@ -351,19 +348,13 @@ static int qcom_geni_serial_get_char(struct uart_port
*uport)
u32 rx_fifo;
u32 status;
- status = readl_relaxed(uport->membase + SE_GENI_M_IRQ_STATUS);
- writel_relaxed(status, uport->membase + SE_GENI_M_IRQ_CLEAR);
-
- status = readl_relaxed(uport->membase + SE_GENI_S_IRQ_STATUS);
- writel_relaxed(status, uport->membase + SE_GENI_S_IRQ_CLEAR);
+ status = readl(uport->membase + SE_GENI_M_IRQ_STATUS);
+ writel(status, uport->membase + SE_GENI_M_IRQ_CLEAR);
- /*
-* Ensure the writes to clear interrupts is not re-ordered after
-* reading the data.
-*/
- mb();
+ status = readl(uport->membase + SE_GENI_S_IRQ_STATUS);
+ writel(status, uport->membase + SE_GENI_S_IRQ_CLEAR);
- status = readl_relaxed(uport->membase + SE_GENI_RX_FIFO_STATUS);
+ status = readl(uport->membase + SE_GEN
[PATCH] tty: serial: qcom_geni_serial: Fix UART hang
If a serial console write occured while a UART transmit command was
waiting for a done signal then no further data would be sent until
something new kicked the system into gear. If there is already data
waiting in the circular buffer we must re-enable the tx watermark so we
receive the expected interrupts.
Signed-off-by: Ryan Case
---
drivers/tty/serial/qcom_geni_serial.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/tty/serial/qcom_geni_serial.c
b/drivers/tty/serial/qcom_geni_serial.c
index 0c93beb69e73..a694a47747c7 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -442,6 +442,7 @@ static void qcom_geni_serial_console_write(struct console
*co, const char *s,
bool locked = true;
unsigned long flags;
u32 geni_status;
+ u32 irq_en;
WARN_ON(co->index < 0 || co->index >= GENI_UART_CONS_PORTS);
@@ -476,6 +477,13 @@ static void qcom_geni_serial_console_write(struct console
*co, const char *s,
* has been sent, in which case we need to look for done first.
*/
qcom_geni_serial_poll_tx_done(uport);
+
+ if (uart_circ_chars_pending(>state->xmit)) {
+ irq_en = readl_relaxed(uport->membase +
+ SE_GENI_M_IRQ_EN);
+ writel_relaxed(irq_en | M_TX_FIFO_WATERMARK_EN,
+ uport->membase + SE_GENI_M_IRQ_EN);
+ }
}
__qcom_geni_serial_console_write(uport, s, count);
--
2.20.1.415.g653613c723-goog
Re: [PATCH] tty: serial: qcom_geni_serial: Fix wrap around of TX buffer
On Wed, Dec 19, 2018 at 10:50 AM Evan Green wrote:
>
> On Wed, Dec 19, 2018 at 10:17 AM Matthias Kaehlcke wrote:
> >
> > Before commit a1fee899e5bed ("tty: serial: qcom_geni_serial: Fix
> > softlock") the size of TX transfers was limited to the TX FIFO size,
> > and wrap arounds of the UART circular buffer were split into two
> > transfers. With the commit wrap around are allowed within a transfer.
> > The TX FIFO of the geni serial port uses a word size of 4 bytes. In
> > case of a circular buffer wrap within a transfer the driver currently
> > may write an incomplete word to the FIFO, with some bytes containing
> > data from the circular buffer and others being zero. Since the
> > transfer isn't completed yet the zero bytes are sent as if they were
> > actual data.
> >
> > Handle wrap arounds of the TX buffer properly and ensure that words
> > written to the TX FIFO always contain valid data (unless the transfer
> > is completed).
> >
> > Fixes: a1fee899e5bed ("tty: serial: qcom_geni_serial: Fix softlock")
> > Signed-off-by: Matthias Kaehlcke
>
> Oh nice, so this did end up being your problem with corrupt
> characters? Strange though, I still would have expected this bug to
> result in inserted serial characters, rather than edited ones.
> Either way, this looks good to me.
>
> Reviewed-by: Evan Green
UART bluetooth firmware downloads consistently working for me again.
Tested-by: Ryan Case
[PATCH v2] tty: serial: qcom_geni_serial: Remove interrupt storm
Disable M_TX_FIFO_WATERMARK_EN after we've sent all data for a given
transaction so we don't continue to receive a flurry of free space
interrupts while waiting for the M_CMD_DONE notification. Re-enable the
watermark when establishing the next transaction.
Also clear the watermark interrupt after filling the FIFO so we do not
receive notification again prior to actually having free space.
Signed-off-by: Ryan Case
---
Changes in v2:
Addressed Doug's comments
- Avoid M_TX_WATERMARK_EN writes when values already match
- Added note about M_TX_WATERMARK_EN triggering and latching
drivers/tty/serial/qcom_geni_serial.c | 25 +++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c
b/drivers/tty/serial/qcom_geni_serial.c
index 6e38498362ef..0c93beb69e73 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -724,6 +724,7 @@ static void qcom_geni_serial_handle_tx(struct uart_port
*uport, bool done,
size_t pending;
int i;
u32 status;
+ u32 irq_en;
unsigned int chunk;
int tail;
@@ -752,6 +753,11 @@ static void qcom_geni_serial_handle_tx(struct uart_port
*uport, bool done,
if (!port->tx_remaining) {
qcom_geni_serial_setup_tx(uport, pending);
port->tx_remaining = pending;
+
+ irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
+ if (!(irq_en & M_TX_FIFO_WATERMARK_EN))
+ writel_relaxed(irq_en | M_TX_FIFO_WATERMARK_EN,
+ uport->membase + SE_GENI_M_IRQ_EN);
}
remaining = chunk;
@@ -775,7 +781,23 @@ static void qcom_geni_serial_handle_tx(struct uart_port
*uport, bool done,
}
xmit->tail = tail & (UART_XMIT_SIZE - 1);
+
+ /*
+* The tx fifo watermark is level triggered and latched. Though we had
+* cleared it in qcom_geni_serial_isr it will have already reasserted
+* so we must clear it again here after our writes.
+*/
+ writel_relaxed(M_TX_FIFO_WATERMARK_EN,
+ uport->membase + SE_GENI_M_IRQ_CLEAR);
+
out_write_wakeup:
+ if (!port->tx_remaining) {
+ irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
+ if (irq_en & M_TX_FIFO_WATERMARK_EN)
+ writel_relaxed(irq_en & ~M_TX_FIFO_WATERMARK_EN,
+ uport->membase + SE_GENI_M_IRQ_EN);
+ }
+
if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
uart_write_wakeup(uport);
}
@@ -811,8 +833,7 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
tty_insert_flip_char(tport, 0, TTY_OVERRUN);
}
- if (m_irq_status & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN) &&
- m_irq_en & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN))
+ if (m_irq_status & m_irq_en & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN))
qcom_geni_serial_handle_tx(uport, m_irq_status & M_CMD_DONE_EN,
geni_status & M_GENI_CMD_ACTIVE);
--
2.20.0.rc2.403.gdbc3b29805-goog
[PATCH] tty: serial: qcom_geni_serial: Remove interrupt storm
Disable M_TX_FIFO_WATERMARK_EN after we've sent all data for a given
transaction so we don't continue to receive a flurry of free space
interrupts while waiting for the M_CMD_DONE notification. Re-enable the
watermark when establishing the next transaction.
Also clear the watermark interrupt after filling the FIFO so we do not
receive notification again prior to actually having free space.
Signed-off-by: Ryan Case
---
drivers/tty/serial/qcom_geni_serial.c | 16 ++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c
b/drivers/tty/serial/qcom_geni_serial.c
index 6e38498362ef..965aefa54114 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -724,10 +724,12 @@ static void qcom_geni_serial_handle_tx(struct uart_port
*uport, bool done,
size_t pending;
int i;
u32 status;
+ u32 irq_en;
unsigned int chunk;
int tail;
status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
+ irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
/* Complete the current tx command before taking newly added data */
if (active)
@@ -752,6 +754,9 @@ static void qcom_geni_serial_handle_tx(struct uart_port
*uport, bool done,
if (!port->tx_remaining) {
qcom_geni_serial_setup_tx(uport, pending);
port->tx_remaining = pending;
+
+ irq_en |= M_TX_FIFO_WATERMARK_EN;
+ writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
}
remaining = chunk;
@@ -775,7 +780,15 @@ static void qcom_geni_serial_handle_tx(struct uart_port
*uport, bool done,
}
xmit->tail = tail & (UART_XMIT_SIZE - 1);
+ writel_relaxed(M_TX_FIFO_WATERMARK_EN,
+ uport->membase + SE_GENI_M_IRQ_CLEAR);
+
out_write_wakeup:
+ if (!port->tx_remaining) {
+ irq_en &= ~M_TX_FIFO_WATERMARK_EN;
+ writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
+ }
+
if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
uart_write_wakeup(uport);
}
@@ -811,8 +824,7 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
tty_insert_flip_char(tport, 0, TTY_OVERRUN);
}
- if (m_irq_status & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN) &&
- m_irq_en & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN))
+ if (m_irq_status & m_irq_en & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN))
qcom_geni_serial_handle_tx(uport, m_irq_status & M_CMD_DONE_EN,
geni_status & M_GENI_CMD_ACTIVE);
--
2.20.0.rc2.403.gdbc3b29805-goog
[V3] ASoC: max98373: Added max98373_reset for stable amp reset
This patch added max98373_reset function to avoid amp software reset failure
and code duplication.
Reset verification step has been added for stable amp reset and it repeats
verification maximum 3 times when it is failed.
Chip revision ID is available when the amp is in the idle state which means
software reset is completed well.
Additional 10ms delay was added for every retrial and maximum 30ms delay can be
applied.
Signed-off-by: Ryan Lee
---
Changes since v2:
- Added commit message.
Changes since v1:
- Removed unusual repeat for amp software reset and verification.
- Amp software reset will be performed once and it repeats verification maximum
3 times if it is failed.
- Wait 10ms before every verification trial. Maximum 30ms delay will be applied
to wait AMP idle state.
Changes:
- Created max98373_reset function to minimize code duplication.
- Changed regmap_write to regmap_update_bits. Other bits except LSB need to be
masked.
- Added reset verification step to make sure software reset is completed well.
Software reset is done in 10ms in normal case.
- Revision ID is available when the amp is in the idle state which means
software reset is completed.
- Software reset will be performed maximum 3 times to avoid amp reset failure.
Generally it is done in the first trial.
- sleep time after software reset is increased + 30ms for every retrial.
Maximum possible msleep time is 100 ms (initial 10 ms + 30 ms * 3 times).
sound/soc/codecs/max98373.c | 35 +--
1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/sound/soc/codecs/max98373.c b/sound/soc/codecs/max98373.c
index a09d013..9c8616a 100644
--- a/sound/soc/codecs/max98373.c
+++ b/sound/soc/codecs/max98373.c
@@ -724,14 +724,39 @@ static struct snd_soc_dai_driver max98373_dai[] = {
}
};
+static void max98373_reset(struct max98373_priv *max98373, struct device *dev)
+{
+ int ret, reg, count;
+
+ /* Software Reset */
+ ret = regmap_update_bits(max98373->regmap,
+ MAX98373_R2000_SW_RESET,
+ MAX98373_SOFT_RESET,
+ MAX98373_SOFT_RESET);
+ if (ret)
+ dev_err(dev, "Reset command failed. (ret:%d)\n", ret);
+
+ count = 0;
+ while (count < 3) {
+ usleep_range(1, 11000);
+ /* Software Reset Verification */
+ ret = regmap_read(max98373->regmap,
+ MAX98373_R21FF_REV_ID, );
+ if (!ret) {
+ dev_info(dev, "Reset completed (retry:%d)\n", count);
+ return;
+ }
+ count++;
+ }
+ dev_err(dev, "Reset failed. (ret:%d)\n", ret);
+}
+
static int max98373_probe(struct snd_soc_component *component)
{
struct max98373_priv *max98373 =
snd_soc_component_get_drvdata(component);
/* Software Reset */
- regmap_write(max98373->regmap,
- MAX98373_R2000_SW_RESET, MAX98373_SOFT_RESET);
- usleep_range(1, 11000);
+ max98373_reset(max98373, component->dev);
/* IV default slot configuration */
regmap_write(max98373->regmap,
@@ -818,9 +843,7 @@ static int max98373_resume(struct device *dev)
{
struct max98373_priv *max98373 = dev_get_drvdata(dev);
- regmap_write(max98373->regmap,
- MAX98373_R2000_SW_RESET, MAX98373_SOFT_RESET);
- usleep_range(1, 11000);
+ max98373_reset(max98373, dev);
regcache_cache_only(max98373->regmap, false);
regcache_sync(max98373->regmap);
return 0;
--
2.7.4
[V3] ASoC: max98373: Added max98373_reset for stable amp reset
This patch added max98373_reset function to avoid amp software reset failure
and code duplication.
Reset verification step has been added for stable amp reset and it repeats
verification maximum 3 times when it is failed.
Chip revision ID is available when the amp is in the idle state which means
software reset is completed well.
Additional 10ms delay was added for every retrial and maximum 30ms delay can be
applied.
Signed-off-by: Ryan Lee
---
Changes since v2:
- Added commit message.
Changes since v1:
- Removed unusual repeat for amp software reset and verification.
- Amp software reset will be performed once and it repeats verification maximum
3 times if it is failed.
- Wait 10ms before every verification trial. Maximum 30ms delay will be applied
to wait AMP idle state.
Changes:
- Created max98373_reset function to minimize code duplication.
- Changed regmap_write to regmap_update_bits. Other bits except LSB need to be
masked.
- Added reset verification step to make sure software reset is completed well.
Software reset is done in 10ms in normal case.
- Revision ID is available when the amp is in the idle state which means
software reset is completed.
- Software reset will be performed maximum 3 times to avoid amp reset failure.
Generally it is done in the first trial.
- sleep time after software reset is increased + 30ms for every retrial.
Maximum possible msleep time is 100 ms (initial 10 ms + 30 ms * 3 times).
sound/soc/codecs/max98373.c | 35 +--
1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/sound/soc/codecs/max98373.c b/sound/soc/codecs/max98373.c
index a09d013..9c8616a 100644
--- a/sound/soc/codecs/max98373.c
+++ b/sound/soc/codecs/max98373.c
@@ -724,14 +724,39 @@ static struct snd_soc_dai_driver max98373_dai[] = {
}
};
+static void max98373_reset(struct max98373_priv *max98373, struct device *dev)
+{
+ int ret, reg, count;
+
+ /* Software Reset */
+ ret = regmap_update_bits(max98373->regmap,
+ MAX98373_R2000_SW_RESET,
+ MAX98373_SOFT_RESET,
+ MAX98373_SOFT_RESET);
+ if (ret)
+ dev_err(dev, "Reset command failed. (ret:%d)\n", ret);
+
+ count = 0;
+ while (count < 3) {
+ usleep_range(1, 11000);
+ /* Software Reset Verification */
+ ret = regmap_read(max98373->regmap,
+ MAX98373_R21FF_REV_ID, );
+ if (!ret) {
+ dev_info(dev, "Reset completed (retry:%d)\n", count);
+ return;
+ }
+ count++;
+ }
+ dev_err(dev, "Reset failed. (ret:%d)\n", ret);
+}
+
static int max98373_probe(struct snd_soc_component *component)
{
struct max98373_priv *max98373 =
snd_soc_component_get_drvdata(component);
/* Software Reset */
- regmap_write(max98373->regmap,
- MAX98373_R2000_SW_RESET, MAX98373_SOFT_RESET);
- usleep_range(1, 11000);
+ max98373_reset(max98373, component->dev);
/* IV default slot configuration */
regmap_write(max98373->regmap,
@@ -818,9 +843,7 @@ static int max98373_resume(struct device *dev)
{
struct max98373_priv *max98373 = dev_get_drvdata(dev);
- regmap_write(max98373->regmap,
- MAX98373_R2000_SW_RESET, MAX98373_SOFT_RESET);
- usleep_range(1, 11000);
+ max98373_reset(max98373, dev);
regcache_cache_only(max98373->regmap, false);
regcache_sync(max98373->regmap);
return 0;
--
2.7.4
RE: [PATCH V2] ASoC: max98373: Added max98373_reset for stable amp reset
>-Original Message- >From: Mark Brown >Sent: Thursday, November 29, 2018 3:26 AM >To: Grant Grundler >Cc: Ryan Lee ; Liam Girdwood >; pe...@perex.cz; ti...@suse.com; Kuninori >Morimoto ; Benson Leung >; alsa-de...@alsa-project.org; LKML ker...@vger.kernel.org>; ryan.lee.ma...@gmail.com >Subject: Re: [PATCH V2] ASoC: max98373: Added max98373_reset for stable >amp reset > >On Wed, Nov 28, 2018 at 05:55:48PM -0800, Grant Grundler wrote: >> On Wed, Nov 28, 2018 at 9:07 AM Ryan Lee > wrote: > >> > >Not seeing a changelog here like I asked for :( > >> > Actually I added changelog as below. Do you mean this is not sufficient? > >> The text is probably sufficient but not in a format that Mark can >> directly apply. >> Please take a quick look at Documentation/process/submitting-patches.rst. > >> Mark wants the "commit message" to be before the '---' line. So move >> the "Changes:" text up to become the commit message and drop the >> "Changes" line. That should explain why this commit is needed and >> include the S-o-B line. > >Right. If you compare what's in git and what you're sending with other >commits and mails and make sure everything looks similar you're probably on >the right track. I'm sorry for the mistake. Let me fix it.
RE: [PATCH V2] ASoC: max98373: Added max98373_reset for stable amp reset
>-Original Message- >From: Mark Brown >Sent: Thursday, November 29, 2018 3:26 AM >To: Grant Grundler >Cc: Ryan Lee ; Liam Girdwood >; pe...@perex.cz; ti...@suse.com; Kuninori >Morimoto ; Benson Leung >; alsa-de...@alsa-project.org; LKML ker...@vger.kernel.org>; ryan.lee.ma...@gmail.com >Subject: Re: [PATCH V2] ASoC: max98373: Added max98373_reset for stable >amp reset > >On Wed, Nov 28, 2018 at 05:55:48PM -0800, Grant Grundler wrote: >> On Wed, Nov 28, 2018 at 9:07 AM Ryan Lee > wrote: > >> > >Not seeing a changelog here like I asked for :( > >> > Actually I added changelog as below. Do you mean this is not sufficient? > >> The text is probably sufficient but not in a format that Mark can >> directly apply. >> Please take a quick look at Documentation/process/submitting-patches.rst. > >> Mark wants the "commit message" to be before the '---' line. So move >> the "Changes:" text up to become the commit message and drop the >> "Changes" line. That should explain why this commit is needed and >> include the S-o-B line. > >Right. If you compare what's in git and what you're sending with other >commits and mails and make sure everything looks similar you're probably on >the right track. I'm sorry for the mistake. Let me fix it.
RE: [PATCH V2] ASoC: max98373: Added max98373_reset for stable amp reset
>-Original Message- >From: Grant Grundler >Sent: Wednesday, November 28, 2018 5:56 PM >To: Ryan Lee >Cc: broo...@kernel.org; Liam Girdwood ; >pe...@perex.cz; ti...@suse.com; Grant Grundler >; Kuninori Morimoto >; Benson Leung >; alsa-de...@alsa-project.org; LKML ker...@vger.kernel.org>; ryan.lee.ma...@gmail.com >Subject: Re: [PATCH V2] ASoC: max98373: Added max98373_reset for stable >amp reset > >EXTERNAL EMAIL > > > >On Wed, Nov 28, 2018 at 9:07 AM Ryan Lee > wrote: >> >> >-Original Message- >> >From: Mark Brown >> >Sent: Wednesday, November 28, 2018 1:50 AM >> >To: Ryan Lee >> >Cc: Liam Girdwood ; Jaroslav Kysela >> >; Takashi Iwai ; Grant Grundler >> >; Kuninori Morimoto >> >; Benson Leung >> >; alsa-de...@alsa-project.org; linux- >> >ker...@vger.kernel.org; ryan.lee.ma...@gmail.com >> >Subject: Re: [PATCH V2] ASoC: max98373: Added max98373_reset for >> >stable amp reset >> > >> >On Wed, Nov 28, 2018 at 03:20:16AM +, Ryan Lee wrote: >> >> Signed-off-by: Ryan Lee >> >> --- >> > >> >Not seeing a changelog here like I asked for :( >> >> Actually I added changelog as below. Do you mean this is not sufficient? > >The text is probably sufficient but not in a format that Mark can directly >apply. >Please take a quick look at Documentation/process/submitting-patches.rst. > >Mark wants the "commit message" to be before the '---' line. So move the >"Changes:" text up to become the commit message and drop the "Changes" >line. That should explain why this commit is needed and include the S-o-B line. Thanks for your help. I will fix it. > >cheers, >grant > >> >> Changes since v1 : Removed unusual repeat for amp software reset and >verification. >>Amp software reset will be performed once and it repeats >verification maximum 3 times if it is failed. >>Wait 10ms before every verification trial. Maximum 30ms >> delay will >be applied to wait AMP idle state. >> >> > >> >> Changes : Created max98373_reset function to minimize code duplication. >> >> Changed regmap_write to regmap_update_bits. Other bits >> >> except LSB >> >need to be masked. >> >> Added reset verification step to make sure software reset >> >> is completed >> >well. Software reset is done in 10ms in normal case. >> >> Revision ID is available when the amp is in the idle >> >> state which means >> >software reset is completed. >> >> Software reset will be performed maximum 3 times to avoid >> >> amp reset >> >failure. Generally it is done in the first trial. >> >> sleep time after software reset is increased + 30ms for every >> >> retrial. >> >Maximum possible msleep time is 100 ms (initial 10 ms + 30 ms * 3 times). >> > >> >This looks like it's supposed to be a changelog but it isn't one?
RE: [PATCH V2] ASoC: max98373: Added max98373_reset for stable amp reset
>-Original Message- >From: Grant Grundler >Sent: Wednesday, November 28, 2018 5:56 PM >To: Ryan Lee >Cc: broo...@kernel.org; Liam Girdwood ; >pe...@perex.cz; ti...@suse.com; Grant Grundler >; Kuninori Morimoto >; Benson Leung >; alsa-de...@alsa-project.org; LKML ker...@vger.kernel.org>; ryan.lee.ma...@gmail.com >Subject: Re: [PATCH V2] ASoC: max98373: Added max98373_reset for stable >amp reset > >EXTERNAL EMAIL > > > >On Wed, Nov 28, 2018 at 9:07 AM Ryan Lee > wrote: >> >> >-Original Message- >> >From: Mark Brown >> >Sent: Wednesday, November 28, 2018 1:50 AM >> >To: Ryan Lee >> >Cc: Liam Girdwood ; Jaroslav Kysela >> >; Takashi Iwai ; Grant Grundler >> >; Kuninori Morimoto >> >; Benson Leung >> >; alsa-de...@alsa-project.org; linux- >> >ker...@vger.kernel.org; ryan.lee.ma...@gmail.com >> >Subject: Re: [PATCH V2] ASoC: max98373: Added max98373_reset for >> >stable amp reset >> > >> >On Wed, Nov 28, 2018 at 03:20:16AM +, Ryan Lee wrote: >> >> Signed-off-by: Ryan Lee >> >> --- >> > >> >Not seeing a changelog here like I asked for :( >> >> Actually I added changelog as below. Do you mean this is not sufficient? > >The text is probably sufficient but not in a format that Mark can directly >apply. >Please take a quick look at Documentation/process/submitting-patches.rst. > >Mark wants the "commit message" to be before the '---' line. So move the >"Changes:" text up to become the commit message and drop the "Changes" >line. That should explain why this commit is needed and include the S-o-B line. Thanks for your help. I will fix it. > >cheers, >grant > >> >> Changes since v1 : Removed unusual repeat for amp software reset and >verification. >>Amp software reset will be performed once and it repeats >verification maximum 3 times if it is failed. >>Wait 10ms before every verification trial. Maximum 30ms >> delay will >be applied to wait AMP idle state. >> >> > >> >> Changes : Created max98373_reset function to minimize code duplication. >> >> Changed regmap_write to regmap_update_bits. Other bits >> >> except LSB >> >need to be masked. >> >> Added reset verification step to make sure software reset >> >> is completed >> >well. Software reset is done in 10ms in normal case. >> >> Revision ID is available when the amp is in the idle >> >> state which means >> >software reset is completed. >> >> Software reset will be performed maximum 3 times to avoid >> >> amp reset >> >failure. Generally it is done in the first trial. >> >> sleep time after software reset is increased + 30ms for every >> >> retrial. >> >Maximum possible msleep time is 100 ms (initial 10 ms + 30 ms * 3 times). >> > >> >This looks like it's supposed to be a changelog but it isn't one?
[PATCH v3] tty: serial: qcom_geni_serial: Fix softlock
Transfers were being divided into device FIFO sized (64 byte max)
operations which would poll for completion within a spin_lock_irqsave /
spin_unlock_irqrestore block. This both made things slow by waiting for
the FIFO to completely drain before adding further data and would also
result in softlocks on large transmissions.
This patch allows larger transfers with continuous FIFO additions as
space becomes available and removes polling from the interrupt handler.
Signed-off-by: Ryan Case
---
Changes in v3:
- Reworded comment for clarity
- Fixed mips compiler warning
Changes in v2:
- Addressed nits raised by Stephen
drivers/tty/serial/qcom_geni_serial.c | 56 +++
1 file changed, 39 insertions(+), 17 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c
b/drivers/tty/serial/qcom_geni_serial.c
index 7ded51081add..bbc7d7a5b8b6 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -117,6 +117,8 @@ struct qcom_geni_serial_port {
u32 *rx_fifo;
u32 loopback;
bool brk;
+
+ unsigned int tx_remaining;
};
static const struct uart_ops qcom_geni_console_pops;
@@ -439,6 +441,7 @@ static void qcom_geni_serial_console_write(struct console
*co, const char *s,
struct qcom_geni_serial_port *port;
bool locked = true;
unsigned long flags;
+ u32 geni_status;
WARN_ON(co->index < 0 || co->index >= GENI_UART_CONS_PORTS);
@@ -452,6 +455,8 @@ static void qcom_geni_serial_console_write(struct console
*co, const char *s,
else
spin_lock_irqsave(>lock, flags);
+ geni_status = readl_relaxed(uport->membase + SE_GENI_STATUS);
+
/* Cancel the current write to log the fault */
if (!locked) {
geni_se_cancel_m_cmd(>se);
@@ -465,9 +470,19 @@ static void qcom_geni_serial_console_write(struct console
*co, const char *s,
}
writel_relaxed(M_CMD_CANCEL_EN, uport->membase +
SE_GENI_M_IRQ_CLEAR);
+ } else if ((geni_status & M_GENI_CMD_ACTIVE) && !port->tx_remaining) {
+ /*
+* It seems we can't interrupt existing transfers if all data
+* has been sent, in which case we need to look for done first.
+*/
+ qcom_geni_serial_poll_tx_done(uport);
}
__qcom_geni_serial_console_write(uport, s, count);
+
+ if (port->tx_remaining)
+ qcom_geni_serial_setup_tx(uport, port->tx_remaining);
+
if (locked)
spin_unlock_irqrestore(>lock, flags);
}
@@ -701,40 +716,45 @@ static void qcom_geni_serial_handle_rx(struct uart_port
*uport, bool drop)
port->handle_rx(uport, total_bytes, drop);
}
-static void qcom_geni_serial_handle_tx(struct uart_port *uport)
+static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
+ bool active)
{
struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
struct circ_buf *xmit = >state->xmit;
size_t avail;
size_t remaining;
+ size_t pending;
int i;
u32 status;
unsigned int chunk;
int tail;
- u32 irq_en;
- chunk = uart_circ_chars_pending(xmit);
status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
- /* Both FIFO and framework buffer are drained */
- if (!chunk && !status) {
+
+ /* Complete the current tx command before taking newly added data */
+ if (active)
+ pending = port->tx_remaining;
+ else
+ pending = uart_circ_chars_pending(xmit);
+
+ /* All data has been transmitted and acknowledged as received */
+ if (!pending && !status && done) {
qcom_geni_serial_stop_tx(uport);
goto out_write_wakeup;
}
- if (!uart_console(uport)) {
- irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
- irq_en &= ~(M_TX_FIFO_WATERMARK_EN);
- writel_relaxed(0, uport->membase + SE_GENI_TX_WATERMARK_REG);
- writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
- }
+ avail = port->tx_fifo_depth - (status & TX_FIFO_WC);
+ avail *= port->tx_bytes_pw;
- avail = (port->tx_fifo_depth - port->tx_wm) * port->tx_bytes_pw;
tail = xmit->tail;
- chunk = min3((size_t)chunk, (size_t)(UART_XMIT_SIZE - tail), avail);
+ chunk = min3(avail, pending, (size_t)(UART_XMIT_SIZE - tail));
if (!chunk)
goto out_write_wakeup;
- qcom_geni_serial_setup_tx(uport, chunk);
+ if (!port->tx_remaining) {
+ qcom_geni_serial_setup_tx(uport, pending);
+ port->tx_
[PATCH v3] tty: serial: qcom_geni_serial: Fix softlock
Transfers were being divided into device FIFO sized (64 byte max)
operations which would poll for completion within a spin_lock_irqsave /
spin_unlock_irqrestore block. This both made things slow by waiting for
the FIFO to completely drain before adding further data and would also
result in softlocks on large transmissions.
This patch allows larger transfers with continuous FIFO additions as
space becomes available and removes polling from the interrupt handler.
Signed-off-by: Ryan Case
---
Changes in v3:
- Reworded comment for clarity
- Fixed mips compiler warning
Changes in v2:
- Addressed nits raised by Stephen
drivers/tty/serial/qcom_geni_serial.c | 56 +++
1 file changed, 39 insertions(+), 17 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c
b/drivers/tty/serial/qcom_geni_serial.c
index 7ded51081add..bbc7d7a5b8b6 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -117,6 +117,8 @@ struct qcom_geni_serial_port {
u32 *rx_fifo;
u32 loopback;
bool brk;
+
+ unsigned int tx_remaining;
};
static const struct uart_ops qcom_geni_console_pops;
@@ -439,6 +441,7 @@ static void qcom_geni_serial_console_write(struct console
*co, const char *s,
struct qcom_geni_serial_port *port;
bool locked = true;
unsigned long flags;
+ u32 geni_status;
WARN_ON(co->index < 0 || co->index >= GENI_UART_CONS_PORTS);
@@ -452,6 +455,8 @@ static void qcom_geni_serial_console_write(struct console
*co, const char *s,
else
spin_lock_irqsave(>lock, flags);
+ geni_status = readl_relaxed(uport->membase + SE_GENI_STATUS);
+
/* Cancel the current write to log the fault */
if (!locked) {
geni_se_cancel_m_cmd(>se);
@@ -465,9 +470,19 @@ static void qcom_geni_serial_console_write(struct console
*co, const char *s,
}
writel_relaxed(M_CMD_CANCEL_EN, uport->membase +
SE_GENI_M_IRQ_CLEAR);
+ } else if ((geni_status & M_GENI_CMD_ACTIVE) && !port->tx_remaining) {
+ /*
+* It seems we can't interrupt existing transfers if all data
+* has been sent, in which case we need to look for done first.
+*/
+ qcom_geni_serial_poll_tx_done(uport);
}
__qcom_geni_serial_console_write(uport, s, count);
+
+ if (port->tx_remaining)
+ qcom_geni_serial_setup_tx(uport, port->tx_remaining);
+
if (locked)
spin_unlock_irqrestore(>lock, flags);
}
@@ -701,40 +716,45 @@ static void qcom_geni_serial_handle_rx(struct uart_port
*uport, bool drop)
port->handle_rx(uport, total_bytes, drop);
}
-static void qcom_geni_serial_handle_tx(struct uart_port *uport)
+static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
+ bool active)
{
struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
struct circ_buf *xmit = >state->xmit;
size_t avail;
size_t remaining;
+ size_t pending;
int i;
u32 status;
unsigned int chunk;
int tail;
- u32 irq_en;
- chunk = uart_circ_chars_pending(xmit);
status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
- /* Both FIFO and framework buffer are drained */
- if (!chunk && !status) {
+
+ /* Complete the current tx command before taking newly added data */
+ if (active)
+ pending = port->tx_remaining;
+ else
+ pending = uart_circ_chars_pending(xmit);
+
+ /* All data has been transmitted and acknowledged as received */
+ if (!pending && !status && done) {
qcom_geni_serial_stop_tx(uport);
goto out_write_wakeup;
}
- if (!uart_console(uport)) {
- irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
- irq_en &= ~(M_TX_FIFO_WATERMARK_EN);
- writel_relaxed(0, uport->membase + SE_GENI_TX_WATERMARK_REG);
- writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
- }
+ avail = port->tx_fifo_depth - (status & TX_FIFO_WC);
+ avail *= port->tx_bytes_pw;
- avail = (port->tx_fifo_depth - port->tx_wm) * port->tx_bytes_pw;
tail = xmit->tail;
- chunk = min3((size_t)chunk, (size_t)(UART_XMIT_SIZE - tail), avail);
+ chunk = min3(avail, pending, (size_t)(UART_XMIT_SIZE - tail));
if (!chunk)
goto out_write_wakeup;
- qcom_geni_serial_setup_tx(uport, chunk);
+ if (!port->tx_remaining) {
+ qcom_geni_serial_setup_tx(uport, pending);
+ port->tx_
Re: [PATCH v2] tty: serial: qcom_geni_serial: Fix softlock
On Thu, Nov 29, 2018 at 2:12 PM Doug Anderson wrote:
>
> Hi,
>
> On Wed, Nov 28, 2018 at 3:55 PM Ryan Case wrote:
> > @@ -465,9 +470,19 @@ static void qcom_geni_serial_console_write(struct
> > console *co, const char *s,
> > }
> > writel_relaxed(M_CMD_CANCEL_EN, uport->membase +
> >
> > SE_GENI_M_IRQ_CLEAR);
> > + } else if ((geni_status & M_GENI_CMD_ACTIVE) &&
> > !port->tx_remaining) {
> > + /*
> > +* It seems we can interrupt existing transfers unless all
> > data
>
> s/It seems we can interrupt/It seems we can't interrupt/
Comment is correct as is, but will reword to make things clearer.
>
>
> > +static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
> > + bool active)
> > {
> > struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> > struct circ_buf *xmit = >state->xmit;
> > size_t avail;
> > size_t remaining;
> > + size_t pending;
> > int i;
> > u32 status;
> > unsigned int chunk;
> > int tail;
> > - u32 irq_en;
> >
> > - chunk = uart_circ_chars_pending(xmit);
> > status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
> > - /* Both FIFO and framework buffer are drained */
> > - if (!chunk && !status) {
> > +
> > + /* Complete the current tx command before taking newly added data */
> > + if (active)
> > + pending = port->tx_remaining;
>
> I almost feel like this should be:
>
> if (port->tx_remaining)
> pending = port->tx_remaining
>
> I could imagine active being false but "port->tx_remaining" being
> non-zero if we happened to take a long time to handle the interrupt
> for some reason. Presumably you could simulator this and see what
> breaks. I think what would happen would be "pending" will be larger
> than you expect and you could write a few extra bytes into the FIFO
> causing it to go beyond the length of the transfer you setup. ...so I
> guess you'd drop some bytes?
>
>
> If it's somehow important for "pending" to be 0 still when we're
> active but port->tx_remaining is non-zero, then I guess you could also
> write it as:
>
> if (active || port->tx_remaining)
> pending = port->tx_remaining
>
>
> Maybe I'm misunderstanding though.
Yeah, this flag has different behavior on the hardware than you're
expecting. Active will be set for the duration of the command no
matter the size of the transfer or across how many interrupts the
transfer takes, including after we're done on our side
(port->tx_remaining is zero).
>
>
> -Doug
Re: [PATCH v2] tty: serial: qcom_geni_serial: Fix softlock
On Thu, Nov 29, 2018 at 2:12 PM Doug Anderson wrote:
>
> Hi,
>
> On Wed, Nov 28, 2018 at 3:55 PM Ryan Case wrote:
> > @@ -465,9 +470,19 @@ static void qcom_geni_serial_console_write(struct
> > console *co, const char *s,
> > }
> > writel_relaxed(M_CMD_CANCEL_EN, uport->membase +
> >
> > SE_GENI_M_IRQ_CLEAR);
> > + } else if ((geni_status & M_GENI_CMD_ACTIVE) &&
> > !port->tx_remaining) {
> > + /*
> > +* It seems we can interrupt existing transfers unless all
> > data
>
> s/It seems we can interrupt/It seems we can't interrupt/
Comment is correct as is, but will reword to make things clearer.
>
>
> > +static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
> > + bool active)
> > {
> > struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> > struct circ_buf *xmit = >state->xmit;
> > size_t avail;
> > size_t remaining;
> > + size_t pending;
> > int i;
> > u32 status;
> > unsigned int chunk;
> > int tail;
> > - u32 irq_en;
> >
> > - chunk = uart_circ_chars_pending(xmit);
> > status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
> > - /* Both FIFO and framework buffer are drained */
> > - if (!chunk && !status) {
> > +
> > + /* Complete the current tx command before taking newly added data */
> > + if (active)
> > + pending = port->tx_remaining;
>
> I almost feel like this should be:
>
> if (port->tx_remaining)
> pending = port->tx_remaining
>
> I could imagine active being false but "port->tx_remaining" being
> non-zero if we happened to take a long time to handle the interrupt
> for some reason. Presumably you could simulator this and see what
> breaks. I think what would happen would be "pending" will be larger
> than you expect and you could write a few extra bytes into the FIFO
> causing it to go beyond the length of the transfer you setup. ...so I
> guess you'd drop some bytes?
>
>
> If it's somehow important for "pending" to be 0 still when we're
> active but port->tx_remaining is non-zero, then I guess you could also
> write it as:
>
> if (active || port->tx_remaining)
> pending = port->tx_remaining
>
>
> Maybe I'm misunderstanding though.
Yeah, this flag has different behavior on the hardware than you're
expecting. Active will be set for the duration of the command no
matter the size of the transfer or across how many interrupts the
transfer takes, including after we're done on our side
(port->tx_remaining is zero).
>
>
> -Doug
[PATCH v2] tty: serial: qcom_geni_serial: Fix softlock
Transfers were being divided into device FIFO sized (64 byte max)
operations which would poll for completion within a spin_lock_irqsave /
spin_unlock_irqrestore block. This both made things slow by waiting for
the FIFO to completely drain before adding further data and would also
result in softlocks on large transmissions.
This patch allows larger transfers with continuous FIFO additions as
space becomes available and removes polling from the interrupt handler.
Signed-off-by: Ryan Case
---
Changes in v2:
- Addressed nits raised by Stephen
drivers/tty/serial/qcom_geni_serial.c | 56 +++
1 file changed, 39 insertions(+), 17 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c
b/drivers/tty/serial/qcom_geni_serial.c
index 7ded51081add..26f7c0df83ae 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -117,6 +117,8 @@ struct qcom_geni_serial_port {
u32 *rx_fifo;
u32 loopback;
bool brk;
+
+ unsigned int tx_remaining;
};
static const struct uart_ops qcom_geni_console_pops;
@@ -439,6 +441,7 @@ static void qcom_geni_serial_console_write(struct console
*co, const char *s,
struct qcom_geni_serial_port *port;
bool locked = true;
unsigned long flags;
+ u32 geni_status;
WARN_ON(co->index < 0 || co->index >= GENI_UART_CONS_PORTS);
@@ -452,6 +455,8 @@ static void qcom_geni_serial_console_write(struct console
*co, const char *s,
else
spin_lock_irqsave(>lock, flags);
+ geni_status = readl_relaxed(uport->membase + SE_GENI_STATUS);
+
/* Cancel the current write to log the fault */
if (!locked) {
geni_se_cancel_m_cmd(>se);
@@ -465,9 +470,19 @@ static void qcom_geni_serial_console_write(struct console
*co, const char *s,
}
writel_relaxed(M_CMD_CANCEL_EN, uport->membase +
SE_GENI_M_IRQ_CLEAR);
+ } else if ((geni_status & M_GENI_CMD_ACTIVE) && !port->tx_remaining) {
+ /*
+* It seems we can interrupt existing transfers unless all data
+* has been sent, in which case we need to look for done first.
+*/
+ qcom_geni_serial_poll_tx_done(uport);
}
__qcom_geni_serial_console_write(uport, s, count);
+
+ if (port->tx_remaining)
+ qcom_geni_serial_setup_tx(uport, port->tx_remaining);
+
if (locked)
spin_unlock_irqrestore(>lock, flags);
}
@@ -701,40 +716,45 @@ static void qcom_geni_serial_handle_rx(struct uart_port
*uport, bool drop)
port->handle_rx(uport, total_bytes, drop);
}
-static void qcom_geni_serial_handle_tx(struct uart_port *uport)
+static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
+ bool active)
{
struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
struct circ_buf *xmit = >state->xmit;
size_t avail;
size_t remaining;
+ size_t pending;
int i;
u32 status;
unsigned int chunk;
int tail;
- u32 irq_en;
- chunk = uart_circ_chars_pending(xmit);
status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
- /* Both FIFO and framework buffer are drained */
- if (!chunk && !status) {
+
+ /* Complete the current tx command before taking newly added data */
+ if (active)
+ pending = port->tx_remaining;
+ else
+ pending = uart_circ_chars_pending(xmit);
+
+ /* All data has been transmitted and acknowledged as received */
+ if (!pending && !status && done) {
qcom_geni_serial_stop_tx(uport);
goto out_write_wakeup;
}
- if (!uart_console(uport)) {
- irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
- irq_en &= ~(M_TX_FIFO_WATERMARK_EN);
- writel_relaxed(0, uport->membase + SE_GENI_TX_WATERMARK_REG);
- writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
- }
+ avail = port->tx_fifo_depth - (status & TX_FIFO_WC);
+ avail *= port->tx_bytes_pw;
- avail = (port->tx_fifo_depth - port->tx_wm) * port->tx_bytes_pw;
tail = xmit->tail;
- chunk = min3((size_t)chunk, (size_t)(UART_XMIT_SIZE - tail), avail);
+ chunk = min3(avail, pending, (UART_XMIT_SIZE - tail));
if (!chunk)
goto out_write_wakeup;
- qcom_geni_serial_setup_tx(uport, chunk);
+ if (!port->tx_remaining) {
+ qcom_geni_serial_setup_tx(uport, pending);
+ port->tx_remaining = pending;
+ }
remaining = chunk;
for (i = 0; i <
[PATCH v2] tty: serial: qcom_geni_serial: Fix softlock
Transfers were being divided into device FIFO sized (64 byte max)
operations which would poll for completion within a spin_lock_irqsave /
spin_unlock_irqrestore block. This both made things slow by waiting for
the FIFO to completely drain before adding further data and would also
result in softlocks on large transmissions.
This patch allows larger transfers with continuous FIFO additions as
space becomes available and removes polling from the interrupt handler.
Signed-off-by: Ryan Case
---
Changes in v2:
- Addressed nits raised by Stephen
drivers/tty/serial/qcom_geni_serial.c | 56 +++
1 file changed, 39 insertions(+), 17 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c
b/drivers/tty/serial/qcom_geni_serial.c
index 7ded51081add..26f7c0df83ae 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -117,6 +117,8 @@ struct qcom_geni_serial_port {
u32 *rx_fifo;
u32 loopback;
bool brk;
+
+ unsigned int tx_remaining;
};
static const struct uart_ops qcom_geni_console_pops;
@@ -439,6 +441,7 @@ static void qcom_geni_serial_console_write(struct console
*co, const char *s,
struct qcom_geni_serial_port *port;
bool locked = true;
unsigned long flags;
+ u32 geni_status;
WARN_ON(co->index < 0 || co->index >= GENI_UART_CONS_PORTS);
@@ -452,6 +455,8 @@ static void qcom_geni_serial_console_write(struct console
*co, const char *s,
else
spin_lock_irqsave(>lock, flags);
+ geni_status = readl_relaxed(uport->membase + SE_GENI_STATUS);
+
/* Cancel the current write to log the fault */
if (!locked) {
geni_se_cancel_m_cmd(>se);
@@ -465,9 +470,19 @@ static void qcom_geni_serial_console_write(struct console
*co, const char *s,
}
writel_relaxed(M_CMD_CANCEL_EN, uport->membase +
SE_GENI_M_IRQ_CLEAR);
+ } else if ((geni_status & M_GENI_CMD_ACTIVE) && !port->tx_remaining) {
+ /*
+* It seems we can interrupt existing transfers unless all data
+* has been sent, in which case we need to look for done first.
+*/
+ qcom_geni_serial_poll_tx_done(uport);
}
__qcom_geni_serial_console_write(uport, s, count);
+
+ if (port->tx_remaining)
+ qcom_geni_serial_setup_tx(uport, port->tx_remaining);
+
if (locked)
spin_unlock_irqrestore(>lock, flags);
}
@@ -701,40 +716,45 @@ static void qcom_geni_serial_handle_rx(struct uart_port
*uport, bool drop)
port->handle_rx(uport, total_bytes, drop);
}
-static void qcom_geni_serial_handle_tx(struct uart_port *uport)
+static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
+ bool active)
{
struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
struct circ_buf *xmit = >state->xmit;
size_t avail;
size_t remaining;
+ size_t pending;
int i;
u32 status;
unsigned int chunk;
int tail;
- u32 irq_en;
- chunk = uart_circ_chars_pending(xmit);
status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
- /* Both FIFO and framework buffer are drained */
- if (!chunk && !status) {
+
+ /* Complete the current tx command before taking newly added data */
+ if (active)
+ pending = port->tx_remaining;
+ else
+ pending = uart_circ_chars_pending(xmit);
+
+ /* All data has been transmitted and acknowledged as received */
+ if (!pending && !status && done) {
qcom_geni_serial_stop_tx(uport);
goto out_write_wakeup;
}
- if (!uart_console(uport)) {
- irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
- irq_en &= ~(M_TX_FIFO_WATERMARK_EN);
- writel_relaxed(0, uport->membase + SE_GENI_TX_WATERMARK_REG);
- writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
- }
+ avail = port->tx_fifo_depth - (status & TX_FIFO_WC);
+ avail *= port->tx_bytes_pw;
- avail = (port->tx_fifo_depth - port->tx_wm) * port->tx_bytes_pw;
tail = xmit->tail;
- chunk = min3((size_t)chunk, (size_t)(UART_XMIT_SIZE - tail), avail);
+ chunk = min3(avail, pending, (UART_XMIT_SIZE - tail));
if (!chunk)
goto out_write_wakeup;
- qcom_geni_serial_setup_tx(uport, chunk);
+ if (!port->tx_remaining) {
+ qcom_geni_serial_setup_tx(uport, pending);
+ port->tx_remaining = pending;
+ }
remaining = chunk;
for (i = 0; i <
RE: [PATCH V2] ASoC: max98373: Added max98373_reset for stable amp reset
>-Original Message- >From: Mark Brown >Sent: Wednesday, November 28, 2018 1:50 AM >To: Ryan Lee >Cc: Liam Girdwood ; Jaroslav Kysela >; Takashi Iwai ; Grant Grundler >; Kuninori Morimoto >; Benson Leung >; alsa-de...@alsa-project.org; linux- >ker...@vger.kernel.org; ryan.lee.ma...@gmail.com >Subject: Re: [PATCH V2] ASoC: max98373: Added max98373_reset for stable >amp reset > >On Wed, Nov 28, 2018 at 03:20:16AM +0000, Ryan Lee wrote: >> Signed-off-by: Ryan Lee >> --- > >Not seeing a changelog here like I asked for :( Actually I added changelog as below. Do you mean this is not sufficient? Changes since v1 : Removed unusual repeat for amp software reset and verification. Amp software reset will be performed once and it repeats verification maximum 3 times if it is failed. Wait 10ms before every verification trial. Maximum 30ms delay will be applied to wait AMP idle state. > >> Changes : Created max98373_reset function to minimize code duplication. >> Changed regmap_write to regmap_update_bits. Other bits except LSB >need to be masked. >> Added reset verification step to make sure software reset is >> completed >well. Software reset is done in 10ms in normal case. >> Revision ID is available when the amp is in the idle state which >> means >software reset is completed. >> Software reset will be performed maximum 3 times to avoid amp reset >failure. Generally it is done in the first trial. >> sleep time after software reset is increased + 30ms for every >> retrial. >Maximum possible msleep time is 100 ms (initial 10 ms + 30 ms * 3 times). > >This looks like it's supposed to be a changelog but it isn't one?
RE: [PATCH V2] ASoC: max98373: Added max98373_reset for stable amp reset
>-Original Message- >From: Mark Brown >Sent: Wednesday, November 28, 2018 1:50 AM >To: Ryan Lee >Cc: Liam Girdwood ; Jaroslav Kysela >; Takashi Iwai ; Grant Grundler >; Kuninori Morimoto >; Benson Leung >; alsa-de...@alsa-project.org; linux- >ker...@vger.kernel.org; ryan.lee.ma...@gmail.com >Subject: Re: [PATCH V2] ASoC: max98373: Added max98373_reset for stable >amp reset > >On Wed, Nov 28, 2018 at 03:20:16AM +0000, Ryan Lee wrote: >> Signed-off-by: Ryan Lee >> --- > >Not seeing a changelog here like I asked for :( Actually I added changelog as below. Do you mean this is not sufficient? Changes since v1 : Removed unusual repeat for amp software reset and verification. Amp software reset will be performed once and it repeats verification maximum 3 times if it is failed. Wait 10ms before every verification trial. Maximum 30ms delay will be applied to wait AMP idle state. > >> Changes : Created max98373_reset function to minimize code duplication. >> Changed regmap_write to regmap_update_bits. Other bits except LSB >need to be masked. >> Added reset verification step to make sure software reset is >> completed >well. Software reset is done in 10ms in normal case. >> Revision ID is available when the amp is in the idle state which >> means >software reset is completed. >> Software reset will be performed maximum 3 times to avoid amp reset >failure. Generally it is done in the first trial. >> sleep time after software reset is increased + 30ms for every >> retrial. >Maximum possible msleep time is 100 ms (initial 10 ms + 30 ms * 3 times). > >This looks like it's supposed to be a changelog but it isn't one?
[PATCH V2] ASoC: max98373: Added max98373_reset for stable amp reset
Signed-off-by: Ryan Lee
---
Changes since v1 : Removed unusual repeat for amp software reset and
verification.
Amp software reset will be performed once and it repeats
verification maximum 3 times if it is failed.
Wait 10ms before every verification trial. Maximum 30ms
delay will be applied to wait AMP idle state.
Changes : Created max98373_reset function to minimize code duplication.
Changed regmap_write to regmap_update_bits. Other bits except LSB
need to be masked.
Added reset verification step to make sure software reset is
completed well. Software reset is done in 10ms in normal case.
Revision ID is available when the amp is in the idle state which
means software reset is completed.
Software reset will be performed maximum 3 times to avoid amp reset
failure. Generally it is done in the first trial.
sleep time after software reset is increased + 30ms for every
retrial. Maximum possible msleep time is 100 ms (initial 10 ms + 30 ms * 3
times).
sound/soc/codecs/max98373.c | 35 +--
1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/sound/soc/codecs/max98373.c b/sound/soc/codecs/max98373.c
index a09d013..9c8616a 100644
--- a/sound/soc/codecs/max98373.c
+++ b/sound/soc/codecs/max98373.c
@@ -724,14 +724,39 @@ static struct snd_soc_dai_driver max98373_dai[] = {
}
};
+static void max98373_reset(struct max98373_priv *max98373, struct device *dev)
+{
+ int ret, reg, count;
+
+ /* Software Reset */
+ ret = regmap_update_bits(max98373->regmap,
+ MAX98373_R2000_SW_RESET,
+ MAX98373_SOFT_RESET,
+ MAX98373_SOFT_RESET);
+ if (ret)
+ dev_err(dev, "Reset command failed. (ret:%d)\n", ret);
+
+ count = 0;
+ while (count < 3) {
+ usleep_range(1, 11000);
+ /* Software Reset Verification */
+ ret = regmap_read(max98373->regmap,
+ MAX98373_R21FF_REV_ID, );
+ if (!ret) {
+ dev_info(dev, "Reset completed (retry:%d)\n", count);
+ return;
+ }
+ count++;
+ }
+ dev_err(dev, "Reset failed. (ret:%d)\n", ret);
+}
+
static int max98373_probe(struct snd_soc_component *component)
{
struct max98373_priv *max98373 =
snd_soc_component_get_drvdata(component);
/* Software Reset */
- regmap_write(max98373->regmap,
- MAX98373_R2000_SW_RESET, MAX98373_SOFT_RESET);
- usleep_range(1, 11000);
+ max98373_reset(max98373, component->dev);
/* IV default slot configuration */
regmap_write(max98373->regmap,
@@ -818,9 +843,7 @@ static int max98373_resume(struct device *dev)
{
struct max98373_priv *max98373 = dev_get_drvdata(dev);
- regmap_write(max98373->regmap,
- MAX98373_R2000_SW_RESET, MAX98373_SOFT_RESET);
- usleep_range(1, 11000);
+ max98373_reset(max98373, dev);
regcache_cache_only(max98373->regmap, false);
regcache_sync(max98373->regmap);
return 0;
--
2.7.4
[PATCH V2] ASoC: max98373: Added max98373_reset for stable amp reset
Signed-off-by: Ryan Lee
---
Changes since v1 : Removed unusual repeat for amp software reset and
verification.
Amp software reset will be performed once and it repeats
verification maximum 3 times if it is failed.
Wait 10ms before every verification trial. Maximum 30ms
delay will be applied to wait AMP idle state.
Changes : Created max98373_reset function to minimize code duplication.
Changed regmap_write to regmap_update_bits. Other bits except LSB
need to be masked.
Added reset verification step to make sure software reset is
completed well. Software reset is done in 10ms in normal case.
Revision ID is available when the amp is in the idle state which
means software reset is completed.
Software reset will be performed maximum 3 times to avoid amp reset
failure. Generally it is done in the first trial.
sleep time after software reset is increased + 30ms for every
retrial. Maximum possible msleep time is 100 ms (initial 10 ms + 30 ms * 3
times).
sound/soc/codecs/max98373.c | 35 +--
1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/sound/soc/codecs/max98373.c b/sound/soc/codecs/max98373.c
index a09d013..9c8616a 100644
--- a/sound/soc/codecs/max98373.c
+++ b/sound/soc/codecs/max98373.c
@@ -724,14 +724,39 @@ static struct snd_soc_dai_driver max98373_dai[] = {
}
};
+static void max98373_reset(struct max98373_priv *max98373, struct device *dev)
+{
+ int ret, reg, count;
+
+ /* Software Reset */
+ ret = regmap_update_bits(max98373->regmap,
+ MAX98373_R2000_SW_RESET,
+ MAX98373_SOFT_RESET,
+ MAX98373_SOFT_RESET);
+ if (ret)
+ dev_err(dev, "Reset command failed. (ret:%d)\n", ret);
+
+ count = 0;
+ while (count < 3) {
+ usleep_range(1, 11000);
+ /* Software Reset Verification */
+ ret = regmap_read(max98373->regmap,
+ MAX98373_R21FF_REV_ID, );
+ if (!ret) {
+ dev_info(dev, "Reset completed (retry:%d)\n", count);
+ return;
+ }
+ count++;
+ }
+ dev_err(dev, "Reset failed. (ret:%d)\n", ret);
+}
+
static int max98373_probe(struct snd_soc_component *component)
{
struct max98373_priv *max98373 =
snd_soc_component_get_drvdata(component);
/* Software Reset */
- regmap_write(max98373->regmap,
- MAX98373_R2000_SW_RESET, MAX98373_SOFT_RESET);
- usleep_range(1, 11000);
+ max98373_reset(max98373, component->dev);
/* IV default slot configuration */
regmap_write(max98373->regmap,
@@ -818,9 +843,7 @@ static int max98373_resume(struct device *dev)
{
struct max98373_priv *max98373 = dev_get_drvdata(dev);
- regmap_write(max98373->regmap,
- MAX98373_R2000_SW_RESET, MAX98373_SOFT_RESET);
- usleep_range(1, 11000);
+ max98373_reset(max98373, dev);
regcache_cache_only(max98373->regmap, false);
regcache_sync(max98373->regmap);
return 0;
--
2.7.4
Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
On Tue, Nov 27, 2018 at 6:04 PM Stephen Boyd wrote:
>
> Quoting Ryan Case (2018-11-27 17:24:44)
> > On Tue, Nov 27, 2018 at 4:20 PM Stephen Boyd wrote:
> > >
> > > Quoting Ryan Case (2018-11-26 18:25:36)
> > > > Transfers were being divided into device FIFO sized (64 byte max)
> > > > operations which would poll for completion within a spin_lock_irqsave /
> > > > spin_unlock_irqrestore block. This both made things slow by waiting for
> > > > the FIFO to completely drain before adding further data and would also
> > > > result in softlocks on large transmissions.
> > > >
> > > > This patch allows larger transfers with continuous FIFO additions as
> > > > space becomes available and removes polling from the interrupt handler.
> > > >
> > > > Signed-off-by: Ryan Case
> > > > Version: 1
> > >
> > > I've never seen a Version tag before. Did you manually add this?
> >
> > I submitted with patman, this should have been Series-version:
>
> Hmm ok. I'm not aware of this being a kernel idiom so I would remove
> this tag before sending.
Yup. Series-version: would be properly parsed out and adds the
v1/v2/etc... tags so this won't show up in the v2.
>
> >
> > >
> > > >
> > > > WARN_ON(co->index < 0 || co->index >= GENI_UART_CONS_PORTS);
> > > >
> > > > @@ -465,9 +470,17 @@ static void qcom_geni_serial_console_write(struct
> > > > console *co, const char *s,
> > > > }
> > > > writel_relaxed(M_CMD_CANCEL_EN, uport->membase +
> > > >
> > > > SE_GENI_M_IRQ_CLEAR);
> > > > - }
> > > > + } else if ((geni_status & M_GENI_CMD_ACTIVE) &&
> > > > !port->cur_tx_remaining)
> > > > + /* It seems we can interrupt existing transfers unless
> > > > all data
> > >
> > > Nitpick: Have /* on a line by itself
> > >
> > > Is this comment supposed to say "we can't interrupt existing transfers"?
> >
> > Nope, comment is correct as is.
>
> Ok. I fail at parsing it then. Perhaps
>
> "It seems we can interrupt existing transfers except for when all data
> has been sent"
>
> would make it easier for me to read.
>
> >
> > >
> > > >
> > > > __qcom_geni_serial_console_write(uport, s, count);
> > > > +
> > > > + if (port->cur_tx_remaining)
> > > > + qcom_geni_serial_setup_tx(uport,
> > > > port->cur_tx_remaining);
> > >
> > > Does this happen? Is the console being used as a tty at the same time?
> >
> > Yup, happens quite a bit.
>
> So its being used in both modes at the same time?
Yes.
>
> >
> > >
> > > > +
> > > > if (locked)
> > > > spin_unlock_irqrestore(>lock, flags);
> > > > }
> > > > @@ -701,40 +714,47 @@ static void qcom_geni_serial_handle_rx(struct
> > > > uart_port *uport, bool drop)
> > > > port->handle_rx(uport, total_bytes, drop);
> > > > }
> > > >
> > > > -static void qcom_geni_serial_handle_tx(struct uart_port *uport)
> > > > +static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool
> > > > done,
> > > > + bool active)
> > > > {
> > > > struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> > > > struct circ_buf *xmit = >state->xmit;
> > > > size_t avail;
> > > > size_t remaining;
> > > > + size_t pending;
> > > > int i;
> > > > u32 status;
> > > > unsigned int chunk;
> > > > int tail;
> > > > - u32 irq_en;
> > > >
> > > > - chunk = uart_circ_chars_pending(xmit);
> > > > status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
> > > > - /* Both FIFO and framework buffer are drained */
> > > > - if (!chunk && !status) {
> > > > +
> > > > + /* Complete the current tx command before taking newly added
> > > > data */
> > > > + if (active)
> > > > + pending = port->cur_tx_remaining;
> > &
Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
On Tue, Nov 27, 2018 at 6:04 PM Stephen Boyd wrote:
>
> Quoting Ryan Case (2018-11-27 17:24:44)
> > On Tue, Nov 27, 2018 at 4:20 PM Stephen Boyd wrote:
> > >
> > > Quoting Ryan Case (2018-11-26 18:25:36)
> > > > Transfers were being divided into device FIFO sized (64 byte max)
> > > > operations which would poll for completion within a spin_lock_irqsave /
> > > > spin_unlock_irqrestore block. This both made things slow by waiting for
> > > > the FIFO to completely drain before adding further data and would also
> > > > result in softlocks on large transmissions.
> > > >
> > > > This patch allows larger transfers with continuous FIFO additions as
> > > > space becomes available and removes polling from the interrupt handler.
> > > >
> > > > Signed-off-by: Ryan Case
> > > > Version: 1
> > >
> > > I've never seen a Version tag before. Did you manually add this?
> >
> > I submitted with patman, this should have been Series-version:
>
> Hmm ok. I'm not aware of this being a kernel idiom so I would remove
> this tag before sending.
Yup. Series-version: would be properly parsed out and adds the
v1/v2/etc... tags so this won't show up in the v2.
>
> >
> > >
> > > >
> > > > WARN_ON(co->index < 0 || co->index >= GENI_UART_CONS_PORTS);
> > > >
> > > > @@ -465,9 +470,17 @@ static void qcom_geni_serial_console_write(struct
> > > > console *co, const char *s,
> > > > }
> > > > writel_relaxed(M_CMD_CANCEL_EN, uport->membase +
> > > >
> > > > SE_GENI_M_IRQ_CLEAR);
> > > > - }
> > > > + } else if ((geni_status & M_GENI_CMD_ACTIVE) &&
> > > > !port->cur_tx_remaining)
> > > > + /* It seems we can interrupt existing transfers unless
> > > > all data
> > >
> > > Nitpick: Have /* on a line by itself
> > >
> > > Is this comment supposed to say "we can't interrupt existing transfers"?
> >
> > Nope, comment is correct as is.
>
> Ok. I fail at parsing it then. Perhaps
>
> "It seems we can interrupt existing transfers except for when all data
> has been sent"
>
> would make it easier for me to read.
>
> >
> > >
> > > >
> > > > __qcom_geni_serial_console_write(uport, s, count);
> > > > +
> > > > + if (port->cur_tx_remaining)
> > > > + qcom_geni_serial_setup_tx(uport,
> > > > port->cur_tx_remaining);
> > >
> > > Does this happen? Is the console being used as a tty at the same time?
> >
> > Yup, happens quite a bit.
>
> So its being used in both modes at the same time?
Yes.
>
> >
> > >
> > > > +
> > > > if (locked)
> > > > spin_unlock_irqrestore(>lock, flags);
> > > > }
> > > > @@ -701,40 +714,47 @@ static void qcom_geni_serial_handle_rx(struct
> > > > uart_port *uport, bool drop)
> > > > port->handle_rx(uport, total_bytes, drop);
> > > > }
> > > >
> > > > -static void qcom_geni_serial_handle_tx(struct uart_port *uport)
> > > > +static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool
> > > > done,
> > > > + bool active)
> > > > {
> > > > struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> > > > struct circ_buf *xmit = >state->xmit;
> > > > size_t avail;
> > > > size_t remaining;
> > > > + size_t pending;
> > > > int i;
> > > > u32 status;
> > > > unsigned int chunk;
> > > > int tail;
> > > > - u32 irq_en;
> > > >
> > > > - chunk = uart_circ_chars_pending(xmit);
> > > > status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
> > > > - /* Both FIFO and framework buffer are drained */
> > > > - if (!chunk && !status) {
> > > > +
> > > > + /* Complete the current tx command before taking newly added
> > > > data */
> > > > + if (active)
> > > > + pending = port->cur_tx_remaining;
> > &
Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
On Tue, Nov 27, 2018 at 4:20 PM Stephen Boyd wrote:
>
> Quoting Ryan Case (2018-11-26 18:25:36)
> > Transfers were being divided into device FIFO sized (64 byte max)
> > operations which would poll for completion within a spin_lock_irqsave /
> > spin_unlock_irqrestore block. This both made things slow by waiting for
> > the FIFO to completely drain before adding further data and would also
> > result in softlocks on large transmissions.
> >
> > This patch allows larger transfers with continuous FIFO additions as
> > space becomes available and removes polling from the interrupt handler.
> >
> > Signed-off-by: Ryan Case
> > Version: 1
>
> I've never seen a Version tag before. Did you manually add this?
I submitted with patman, this should have been Series-version:
>
> > diff --git a/drivers/tty/serial/qcom_geni_serial.c
> > b/drivers/tty/serial/qcom_geni_serial.c
> > index 7ded51081add..835a184e0b7d 100644
> > --- a/drivers/tty/serial/qcom_geni_serial.c
> > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > @@ -117,6 +117,8 @@ struct qcom_geni_serial_port {
> > u32 *rx_fifo;
> > u32 loopback;
> > bool brk;
> > +
> > + u32 cur_tx_remaining;
>
> Nitpick: Can it just be tx_remaining? And why u32? Why not unsigned int?
Sure, and unsigned int is fine.
>
> > };
> >
> > static const struct uart_ops qcom_geni_console_pops;
> > @@ -439,6 +441,7 @@ static void qcom_geni_serial_console_write(struct
> > console *co, const char *s,
> > struct qcom_geni_serial_port *port;
> > bool locked = true;
> > unsigned long flags;
> > + unsigned int geni_status;
>
> Nitpick: Use u32 for register reads.
will do.
>
> >
> > WARN_ON(co->index < 0 || co->index >= GENI_UART_CONS_PORTS);
> >
> > @@ -465,9 +470,17 @@ static void qcom_geni_serial_console_write(struct
> > console *co, const char *s,
> > }
> > writel_relaxed(M_CMD_CANCEL_EN, uport->membase +
> >
> > SE_GENI_M_IRQ_CLEAR);
> > - }
> > + } else if ((geni_status & M_GENI_CMD_ACTIVE) &&
> > !port->cur_tx_remaining)
> > + /* It seems we can interrupt existing transfers unless all
> > data
>
> Nitpick: Have /* on a line by itself
>
> Is this comment supposed to say "we can't interrupt existing transfers"?
Nope, comment is correct as is.
>
> > +* has been sent, in which case we need to look for done
> > first.
> > +*/
> > + qcom_geni_serial_poll_tx_done(uport);
>
> Another nitpick: Please put braces around multi-line if branches for
> greater code clarity.
will do.
>
> >
> > __qcom_geni_serial_console_write(uport, s, count);
> > +
> > + if (port->cur_tx_remaining)
> > + qcom_geni_serial_setup_tx(uport, port->cur_tx_remaining);
>
> Does this happen? Is the console being used as a tty at the same time?
Yup, happens quite a bit.
>
> > +
> > if (locked)
> > spin_unlock_irqrestore(>lock, flags);
> > }
> > @@ -701,40 +714,47 @@ static void qcom_geni_serial_handle_rx(struct
> > uart_port *uport, bool drop)
> > port->handle_rx(uport, total_bytes, drop);
> > }
> >
> > -static void qcom_geni_serial_handle_tx(struct uart_port *uport)
> > +static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
> > + bool active)
> > {
> > struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> > struct circ_buf *xmit = >state->xmit;
> > size_t avail;
> > size_t remaining;
> > + size_t pending;
> > int i;
> > u32 status;
> > unsigned int chunk;
> > int tail;
> > - u32 irq_en;
> >
> > - chunk = uart_circ_chars_pending(xmit);
> > status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
> > - /* Both FIFO and framework buffer are drained */
> > - if (!chunk && !status) {
> > +
> > + /* Complete the current tx command before taking newly added data */
> > + if (active)
> > + pending = port->cur_tx_remaining;
> > + else
> > + pending = uart_circ_chars_pending(xmit);
> > +
> > + /* All data has been transmitted and acknowledged as received *
Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
On Tue, Nov 27, 2018 at 4:20 PM Stephen Boyd wrote:
>
> Quoting Ryan Case (2018-11-26 18:25:36)
> > Transfers were being divided into device FIFO sized (64 byte max)
> > operations which would poll for completion within a spin_lock_irqsave /
> > spin_unlock_irqrestore block. This both made things slow by waiting for
> > the FIFO to completely drain before adding further data and would also
> > result in softlocks on large transmissions.
> >
> > This patch allows larger transfers with continuous FIFO additions as
> > space becomes available and removes polling from the interrupt handler.
> >
> > Signed-off-by: Ryan Case
> > Version: 1
>
> I've never seen a Version tag before. Did you manually add this?
I submitted with patman, this should have been Series-version:
>
> > diff --git a/drivers/tty/serial/qcom_geni_serial.c
> > b/drivers/tty/serial/qcom_geni_serial.c
> > index 7ded51081add..835a184e0b7d 100644
> > --- a/drivers/tty/serial/qcom_geni_serial.c
> > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > @@ -117,6 +117,8 @@ struct qcom_geni_serial_port {
> > u32 *rx_fifo;
> > u32 loopback;
> > bool brk;
> > +
> > + u32 cur_tx_remaining;
>
> Nitpick: Can it just be tx_remaining? And why u32? Why not unsigned int?
Sure, and unsigned int is fine.
>
> > };
> >
> > static const struct uart_ops qcom_geni_console_pops;
> > @@ -439,6 +441,7 @@ static void qcom_geni_serial_console_write(struct
> > console *co, const char *s,
> > struct qcom_geni_serial_port *port;
> > bool locked = true;
> > unsigned long flags;
> > + unsigned int geni_status;
>
> Nitpick: Use u32 for register reads.
will do.
>
> >
> > WARN_ON(co->index < 0 || co->index >= GENI_UART_CONS_PORTS);
> >
> > @@ -465,9 +470,17 @@ static void qcom_geni_serial_console_write(struct
> > console *co, const char *s,
> > }
> > writel_relaxed(M_CMD_CANCEL_EN, uport->membase +
> >
> > SE_GENI_M_IRQ_CLEAR);
> > - }
> > + } else if ((geni_status & M_GENI_CMD_ACTIVE) &&
> > !port->cur_tx_remaining)
> > + /* It seems we can interrupt existing transfers unless all
> > data
>
> Nitpick: Have /* on a line by itself
>
> Is this comment supposed to say "we can't interrupt existing transfers"?
Nope, comment is correct as is.
>
> > +* has been sent, in which case we need to look for done
> > first.
> > +*/
> > + qcom_geni_serial_poll_tx_done(uport);
>
> Another nitpick: Please put braces around multi-line if branches for
> greater code clarity.
will do.
>
> >
> > __qcom_geni_serial_console_write(uport, s, count);
> > +
> > + if (port->cur_tx_remaining)
> > + qcom_geni_serial_setup_tx(uport, port->cur_tx_remaining);
>
> Does this happen? Is the console being used as a tty at the same time?
Yup, happens quite a bit.
>
> > +
> > if (locked)
> > spin_unlock_irqrestore(>lock, flags);
> > }
> > @@ -701,40 +714,47 @@ static void qcom_geni_serial_handle_rx(struct
> > uart_port *uport, bool drop)
> > port->handle_rx(uport, total_bytes, drop);
> > }
> >
> > -static void qcom_geni_serial_handle_tx(struct uart_port *uport)
> > +static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
> > + bool active)
> > {
> > struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> > struct circ_buf *xmit = >state->xmit;
> > size_t avail;
> > size_t remaining;
> > + size_t pending;
> > int i;
> > u32 status;
> > unsigned int chunk;
> > int tail;
> > - u32 irq_en;
> >
> > - chunk = uart_circ_chars_pending(xmit);
> > status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
> > - /* Both FIFO and framework buffer are drained */
> > - if (!chunk && !status) {
> > +
> > + /* Complete the current tx command before taking newly added data */
> > + if (active)
> > + pending = port->cur_tx_remaining;
> > + else
> > + pending = uart_circ_chars_pending(xmit);
> > +
> > + /* All data has been transmitted and acknowledged as received *
RE: [PATCH] ASoC: max98373: Added max98373_reset for stable amp reset
>-Original Message- >From: Mark Brown >Sent: Tuesday, November 27, 2018 3:51 AM >To: Ryan Lee >Cc: Liam Girdwood ; Jaroslav Kysela >; Takashi Iwai ; Grant Grundler >; Kuninori Morimoto >; Benson Leung >; alsa-de...@alsa-project.org; linux- >ker...@vger.kernel.org >Subject: Re: [PATCH] ASoC: max98373: Added max98373_reset for stable amp >reset > >On Mon, Nov 26, 2018 at 06:46:05PM +0000, Ryan Lee wrote: >> Signed-off-by: Ryan Lee >> --- > >This really needs a changelog to explain what is going on here, and we need >some more documentation in the code. It is *extremely* unusual to have to >poll for reset like this, and if the failure mode is I/O errors that's going >to be >pretty painful. OK. I agree that this is very unusual. I wanted to make this code change very conservative and this caused unusual overhead. Let me fix this.
RE: [PATCH] ASoC: max98373: Added max98373_reset for stable amp reset
>-Original Message- >From: Mark Brown >Sent: Tuesday, November 27, 2018 3:51 AM >To: Ryan Lee >Cc: Liam Girdwood ; Jaroslav Kysela >; Takashi Iwai ; Grant Grundler >; Kuninori Morimoto >; Benson Leung >; alsa-de...@alsa-project.org; linux- >ker...@vger.kernel.org >Subject: Re: [PATCH] ASoC: max98373: Added max98373_reset for stable amp >reset > >On Mon, Nov 26, 2018 at 06:46:05PM +0000, Ryan Lee wrote: >> Signed-off-by: Ryan Lee >> --- > >This really needs a changelog to explain what is going on here, and we need >some more documentation in the code. It is *extremely* unusual to have to >poll for reset like this, and if the failure mode is I/O errors that's going >to be >pretty painful. OK. I agree that this is very unusual. I wanted to make this code change very conservative and this caused unusual overhead. Let me fix this.
RE: [PATCH] ASoC: max98373: Added max98373_reset for stable amp reset
Grant,
Thanks for your feedback. Please find my answers inline.
>-Original Message-
>From: Grant Grundler
>Sent: Monday, November 26, 2018 6:25 PM
>To: Ryan Lee
>Cc: Liam Girdwood ; broo...@kernel.org;
>pe...@perex.cz; ti...@suse.com; Grant Grundler
>; Kuninori Morimoto
>; Benson Leung
>; alsa-de...@alsa-project.org; LKML ker...@vger.kernel.org>
>Subject: Re: [PATCH] ASoC: max98373: Added max98373_reset for stable amp
>reset
>
>EXTERNAL EMAIL
>
>
>
>Hi Ryan!
>
>Just some questions inline - in general I like the reset function.
>
>On Mon, Nov 26, 2018 at 10:46 AM Ryan Lee
> wrote:
>>
>> Signed-off-by: Ryan Lee
>> ---
>> Changes : Created max98373_reset function to minimize code duplication.
>>Changed regmap_write to regmap_update_bits. Other bits except LSB
>need to be masked.
>>Added reset verification step to make sure software reset is
>completed well. Software reset is done in 10ms in normal case.
>>Revision ID is available when the amp is in the idle state which
>> means
>software reset is completed.
>>Software reset will be performed maximum 3 times to avoid amp
>> reset
>failure. Generally it is done in the first trial.
>>sleep time after software reset is increased + 30ms for every
>> retrial.
>Maximum possible msleep time is 100 ms (initial 10 ms + 30 ms * 3 times).
>
>Why is the sleep time increased after each SW reset?
>What is the failure case that you've seen which would benefit from this?
Generally 10ms is enough time for amp software reset but I wanted to add more
guard time for the retrial because it is already failed once.
I have not seen a failure issue with 10ms delay on my test setup but I wanted
to make it more conservative.
Let me remove this.
>
>>
>> sound/soc/codecs/max98373.c | 41
>> +++--
>> 1 file changed, 35 insertions(+), 6 deletions(-)
>>
>> diff --git a/sound/soc/codecs/max98373.c b/sound/soc/codecs/max98373.c
>> index a09d013..55af7f02 100644
>> --- a/sound/soc/codecs/max98373.c
>> +++ b/sound/soc/codecs/max98373.c
>> @@ -724,14 +724,45 @@ static struct snd_soc_dai_driver max98373_dai[] =
>{
>> }
>> };
>>
>> +static void max98373_reset(struct max98373_priv *max98373, struct
>> +device *dev) {
>> + int ret, reg, count, delay;
>> +
>> + count = 0;
>> + while (true) {
>> + /* Software Reset */
>> + ret = regmap_update_bits(max98373->regmap,
>> + MAX98373_R2000_SW_RESET,
>> + MAX98373_SOFT_RESET,
>> + MAX98373_SOFT_RESET);
>> + if (ret)
>> + dev_err(dev, "Reset command failed.
>> + (ret:%d)\n", ret);
>> +
>> + delay = 1 + (count * 3);
>> + usleep_range(delay, delay + 1000);
>> +
>> + /* Software Reset Verification */
>> + ret = regmap_read(max98373->regmap,
>> + MAX98373_R21FF_REV_ID, );
>> + if (!ret) {
>> + dev_info(dev, "Reset completed (retry:%d)\n", count);
>> + break;
>
>Instead of break, can the code return here?
>"break" implies something else will happen after the while loop exits
>- there isn't.
Okay. I will fix it. Thanks for the comment.
>
>> + }
>> +
>> + if (++count > 3){
>> + dev_err(dev, "Reset failed. (ret:%d)\n", ret);
>> + break;
>> + }
>> + usleep_range(1, 11000);
>
>Why is there a second delay after reading MAX98373_R21FF_REV_ID?
>Is this really necessary?
It is not very necessary. I just wanted to make it more conservative because
this delay is applied only when reset is failed.
>
>If the second usleep_range() isn't needed, it would be better/clearer
>to make code loop on "while (count < 4)". And then outside the while
>loop, use dev_err() to share what the failure was.
OK. Let me fix it.
>
>> + }
>> +}
>> +
>> static int max98373_probe(struct snd_soc_component *component) {
>> struct max98373_priv *max98373 =
>> snd_soc_component_get_drvdata(component);
>>
>> /* Software Reset */
>> - regmap_write(max98373->regmap,
>> - MAX98373_R2000_SW_RESET, MAX98373_SOFT_RESET);
>> - usleep_range(1000
