* Re: [PATCH v8 19/21] soc/tegra: pmc: Configure deep sleep control settings
From: Dmitry Osipenko @ 2019-08-09 13:23 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree, rjw, viresh.kumar, linux-pm
In-Reply-To: <1565308020-31952-20-git-send-email-skomatineni@nvidia.com>
09.08.2019 2:46, Sowjanya Komatineni пишет:
> Tegra210 and prior Tegra chips have deep sleep entry and wakeup related
> timings which are platform specific that should be configured before
> entering into deep sleep.
>
> Below are the timing specific configurations for deep sleep entry and
> wakeup.
> - Core rail power-on stabilization timer
> - OSC clock stabilization timer after SOC rail power is stabilized.
> - Core power off time is the minimum wake delay to keep the system
> in deep sleep state irrespective of any quick wake event.
>
> These values depends on the discharge time of regulators and turn OFF
> time of the PMIC to allow the complete system to finish entering into
> deep sleep state.
>
> These values vary based on the platform design and are specified
> through the device tree.
>
> This patch has implementation to configure these timings which are must
> to have for proper deep sleep and wakeup operations.
>
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
> drivers/soc/tegra/pmc.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index e013ada7e4e9..9a78d8417367 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -88,6 +88,8 @@
>
> #define PMC_CPUPWRGOOD_TIMER 0xc8
> #define PMC_CPUPWROFF_TIMER 0xcc
> +#define PMC_COREPWRGOOD_TIMER 0x3c
> +#define PMC_COREPWROFF_TIMER 0xe0
>
> #define PMC_PWR_DET_VALUE 0xe4
>
> @@ -2277,7 +2279,7 @@ static const struct tegra_pmc_regs tegra20_pmc_regs = {
>
> static void tegra20_pmc_init(struct tegra_pmc *pmc)
> {
> - u32 value;
> + u32 value, osc, pmu, off;
>
> /* Always enable CPU power request */
> value = tegra_pmc_readl(pmc, PMC_CNTRL);
> @@ -2303,6 +2305,15 @@ static void tegra20_pmc_init(struct tegra_pmc *pmc)
> value = tegra_pmc_readl(pmc, PMC_CNTRL);
> value |= PMC_CNTRL_SYSCLK_OE;
> tegra_pmc_writel(pmc, value, PMC_CNTRL);
> +
> + osc = DIV_ROUND_UP(pmc->core_osc_time * 8192, 1000000);
> + pmu = DIV_ROUND_UP(pmc->core_pmu_time * 32768, 1000000);
> + off = DIV_ROUND_UP(pmc->core_off_time * 32768, 1000000);
> + if (osc && pmu)
> + tegra_pmc_writel(pmc, ((osc << 8) & 0xff00) | (pmu & 0xff),
> + PMC_COREPWRGOOD_TIMER);
> + if (off)
> + tegra_pmc_writel(pmc, off, PMC_COREPWROFF_TIMER);
The osc/pmu/off values are undefined if they are not defined in device-tree. I suppose this
need to be corrected in tegra_pmc_parse_dt() if the values really matter even if LP0 suspend
isn't supported in device-tree.
And I'm also not sure what's wrong with setting 0 for the timers.
> }
>
> static void tegra20_pmc_setup_irq_polarity(struct tegra_pmc *pmc,
>
^ permalink raw reply
* Re: [PATCH] Add optional chip erase functionality to AT25 EEPROM driver.
From: Joe Burmeister @ 2019-08-09 13:18 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Rob Herring, Mark Rutland, Arnd Bergmann, Srinivas Kandagatla,
YueHaibing, Bartosz Golaszewski, devicetree, linux-kernel
In-Reply-To: <20190809130005.GA13962@kroah.com>
Hi Greg,
On 09/08/2019 14:00, Greg Kroah-Hartman wrote:
> On Fri, Aug 09, 2019 at 01:53:55PM +0100, Joe Burmeister wrote:
>> +static void _eeprom_at25_store_erase_locked(struct at25_data *at25)
>> +{
>> + unsigned long timeout, retries;
>> + int sr, status;
>> + u8 cp;
>> +
>> + cp = AT25_WREN;
>> + status = spi_write(at25->spi, &cp, 1);
>> + if (status < 0) {
>> + dev_dbg(&at25->spi->dev, "ERASE WREN --> %d\n", status);
>> + return;
>> + }
>> + cp = at25->erase_instr;
>> + status = spi_write(at25->spi, &cp, 1);
>> + if (status < 0) {
>> + dev_dbg(&at25->spi->dev, "CHIP_ERASE --> %d\n", status);
>> + return;
>> + }
>> + /* Wait for non-busy status */
>> + timeout = jiffies + msecs_to_jiffies(ERASE_TIMEOUT);
>> + retries = 0;
>> + do {
>> + sr = spi_w8r8(at25->spi, AT25_RDSR);
>> + if (sr < 0 || (sr & AT25_SR_nRDY)) {
>> + dev_dbg(&at25->spi->dev,
>> + "rdsr --> %d (%02x)\n", sr, sr);
>> + /* at HZ=100, this is sloooow */
>> + msleep(1);
>> + continue;
>> + }
>> + if (!(sr & AT25_SR_nRDY))
>> + return;
>> + } while (retries++ < 200 || time_before_eq(jiffies, timeout));
>> +
>> + if ((sr < 0) || (sr & AT25_SR_nRDY)) {
>> + dev_err(&at25->spi->dev,
>> + "chip erase, timeout after %u msecs\n",
>> + jiffies_to_msecs(jiffies -
>> + (timeout - ERASE_TIMEOUT)));
>> + status = -ETIMEDOUT;
>> + return;
>> + }
>> +}
>> +
>> +
> No need for 2 lines :(
Sorry, other coding conventions I'm used to.
>> +static ssize_t eeprom_at25_store_erase(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct at25_data *at25 = dev_get_drvdata(dev);
>> + int erase = 0;
>> +
>> + sscanf(buf, "%d", &erase);
>> + if (erase) {
>> + mutex_lock(&at25->lock);
>> + _eeprom_at25_store_erase_locked(at25);
>> + mutex_unlock(&at25->lock);
>> + }
>> +
>> + return count;
>> +}
>> +
>> +static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_at25_store_erase);
>> +
>> +
> Same here.
>
> Also, where is the Documentation/ABI/ update for the new sysfs file?
There isn't anything for the existing SPI EEPROM stuff I can see.
Would I have to document what was already there to add my bit?
>> static int at25_probe(struct spi_device *spi)
>> {
>> struct at25_data *at25 = NULL;
>> @@ -311,6 +379,7 @@ static int at25_probe(struct spi_device *spi)
>> int err;
>> int sr;
>> int addrlen;
>> + int has_erase;
>>
>> /* Chip description */
>> if (!spi->dev.platform_data) {
>> @@ -352,6 +421,9 @@ static int at25_probe(struct spi_device *spi)
>> spi_set_drvdata(spi, at25);
>> at25->addrlen = addrlen;
>>
>> + /* Optional chip erase instruction */
>> + device_property_read_u8(&spi->dev, "chip_erase_instruction", &at25->erase_instr);
>> +
>> at25->nvmem_config.name = dev_name(&spi->dev);
>> at25->nvmem_config.dev = &spi->dev;
>> at25->nvmem_config.read_only = chip.flags & EE_READONLY;
>> @@ -370,17 +442,22 @@ static int at25_probe(struct spi_device *spi)
>> if (IS_ERR(at25->nvmem))
>> return PTR_ERR(at25->nvmem);
>>
>> - dev_info(&spi->dev, "%d %s %s eeprom%s, pagesize %u\n",
>> + has_erase = (!(chip.flags & EE_READONLY) && at25->erase_instr);
>> +
>> + dev_info(&spi->dev, "%d %s %s eeprom%s, pagesize %u%s\n",
>> (chip.byte_len < 1024) ? chip.byte_len : (chip.byte_len / 1024),
>> (chip.byte_len < 1024) ? "Byte" : "KByte",
>> at25->chip.name,
>> (chip.flags & EE_READONLY) ? " (readonly)" : "",
>> - at25->chip.page_size);
>> + at25->chip.page_size, (has_erase)?" <has erase>":"");
>> +
>> + if (has_erase && device_create_file(&spi->dev, &dev_attr_erase))
>> + dev_err(&spi->dev, "can't create erase interface\n");
> You just raced with userspace and lost :(
>
> Please create an attribute group and add it to the .dev_groups pointer
> in struct driver so it can be created properly in a race-free manner.
>
> See the thread at:
> https://lore.kernel.org/r/20190731124349.4474-2-gregkh@linuxfoundation.org
> for the details on how to do that.
Clearly I didn't know about that. I'll do some reading when I've got a
bit of time and try a again.
> thanks,
>
> greg k-h
Cheers,
Joe
^ permalink raw reply
* Re: [PATCH v8 18/21] soc/tegra: pmc: Configure core power request polarity
From: Dmitry Osipenko @ 2019-08-09 13:13 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree, rjw, viresh.kumar, linux-pm
In-Reply-To: <1565308020-31952-19-git-send-email-skomatineni@nvidia.com>
09.08.2019 2:46, Sowjanya Komatineni пишет:
> This patch configures polarity of the core power request signal
> in PMC control register based on the device tree property.
>
> PMC asserts and de-asserts power request signal based on it polarity
> when it need to power-up and power-down the core rail during SC7.
>
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
> drivers/soc/tegra/pmc.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index 3aa71c28a10a..e013ada7e4e9 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -56,6 +56,7 @@
> #define PMC_CNTRL_SIDE_EFFECT_LP0 BIT(14) /* LP0 when CPU pwr gated */
> #define PMC_CNTRL_SYSCLK_OE BIT(11) /* system clock enable */
> #define PMC_CNTRL_SYSCLK_POLARITY BIT(10) /* sys clk polarity */
> +#define PMC_CNTRL_PWRREQ_POLARITY BIT(8)
> #define PMC_CNTRL_MAIN_RST BIT(4)
>
> #define PMC_WAKE_MASK 0x0c
> @@ -2290,6 +2291,11 @@ static void tegra20_pmc_init(struct tegra_pmc *pmc)
> else
> value |= PMC_CNTRL_SYSCLK_POLARITY;
>
> + if (pmc->corereq_high)
> + value &= ~PMC_CNTRL_PWRREQ_POLARITY;
> + else
> + value |= PMC_CNTRL_PWRREQ_POLARITY;
> +
> /* configure the output polarity while the request is tristated */
> tegra_pmc_writel(pmc, value, PMC_CNTRL);
>
>
Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
^ permalink raw reply
* Re: [PATCH] Add optional chip erase functionality to AT25 EEPROM driver.
From: Greg Kroah-Hartman @ 2019-08-09 13:00 UTC (permalink / raw)
To: Joe Burmeister
Cc: Rob Herring, Mark Rutland, Arnd Bergmann, Srinivas Kandagatla,
YueHaibing, Bartosz Golaszewski, devicetree, linux-kernel
In-Reply-To: <20190809125358.24440-1-joe.burmeister@devtank.co.uk>
On Fri, Aug 09, 2019 at 01:53:55PM +0100, Joe Burmeister wrote:
> +static void _eeprom_at25_store_erase_locked(struct at25_data *at25)
> +{
> + unsigned long timeout, retries;
> + int sr, status;
> + u8 cp;
> +
> + cp = AT25_WREN;
> + status = spi_write(at25->spi, &cp, 1);
> + if (status < 0) {
> + dev_dbg(&at25->spi->dev, "ERASE WREN --> %d\n", status);
> + return;
> + }
> + cp = at25->erase_instr;
> + status = spi_write(at25->spi, &cp, 1);
> + if (status < 0) {
> + dev_dbg(&at25->spi->dev, "CHIP_ERASE --> %d\n", status);
> + return;
> + }
> + /* Wait for non-busy status */
> + timeout = jiffies + msecs_to_jiffies(ERASE_TIMEOUT);
> + retries = 0;
> + do {
> + sr = spi_w8r8(at25->spi, AT25_RDSR);
> + if (sr < 0 || (sr & AT25_SR_nRDY)) {
> + dev_dbg(&at25->spi->dev,
> + "rdsr --> %d (%02x)\n", sr, sr);
> + /* at HZ=100, this is sloooow */
> + msleep(1);
> + continue;
> + }
> + if (!(sr & AT25_SR_nRDY))
> + return;
> + } while (retries++ < 200 || time_before_eq(jiffies, timeout));
> +
> + if ((sr < 0) || (sr & AT25_SR_nRDY)) {
> + dev_err(&at25->spi->dev,
> + "chip erase, timeout after %u msecs\n",
> + jiffies_to_msecs(jiffies -
> + (timeout - ERASE_TIMEOUT)));
> + status = -ETIMEDOUT;
> + return;
> + }
> +}
> +
> +
No need for 2 lines :(
> +static ssize_t eeprom_at25_store_erase(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct at25_data *at25 = dev_get_drvdata(dev);
> + int erase = 0;
> +
> + sscanf(buf, "%d", &erase);
> + if (erase) {
> + mutex_lock(&at25->lock);
> + _eeprom_at25_store_erase_locked(at25);
> + mutex_unlock(&at25->lock);
> + }
> +
> + return count;
> +}
> +
> +static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_at25_store_erase);
> +
> +
Same here.
Also, where is the Documentation/ABI/ update for the new sysfs file?
> static int at25_probe(struct spi_device *spi)
> {
> struct at25_data *at25 = NULL;
> @@ -311,6 +379,7 @@ static int at25_probe(struct spi_device *spi)
> int err;
> int sr;
> int addrlen;
> + int has_erase;
>
> /* Chip description */
> if (!spi->dev.platform_data) {
> @@ -352,6 +421,9 @@ static int at25_probe(struct spi_device *spi)
> spi_set_drvdata(spi, at25);
> at25->addrlen = addrlen;
>
> + /* Optional chip erase instruction */
> + device_property_read_u8(&spi->dev, "chip_erase_instruction", &at25->erase_instr);
> +
> at25->nvmem_config.name = dev_name(&spi->dev);
> at25->nvmem_config.dev = &spi->dev;
> at25->nvmem_config.read_only = chip.flags & EE_READONLY;
> @@ -370,17 +442,22 @@ static int at25_probe(struct spi_device *spi)
> if (IS_ERR(at25->nvmem))
> return PTR_ERR(at25->nvmem);
>
> - dev_info(&spi->dev, "%d %s %s eeprom%s, pagesize %u\n",
> + has_erase = (!(chip.flags & EE_READONLY) && at25->erase_instr);
> +
> + dev_info(&spi->dev, "%d %s %s eeprom%s, pagesize %u%s\n",
> (chip.byte_len < 1024) ? chip.byte_len : (chip.byte_len / 1024),
> (chip.byte_len < 1024) ? "Byte" : "KByte",
> at25->chip.name,
> (chip.flags & EE_READONLY) ? " (readonly)" : "",
> - at25->chip.page_size);
> + at25->chip.page_size, (has_erase)?" <has erase>":"");
> +
> + if (has_erase && device_create_file(&spi->dev, &dev_attr_erase))
> + dev_err(&spi->dev, "can't create erase interface\n");
You just raced with userspace and lost :(
Please create an attribute group and add it to the .dev_groups pointer
in struct driver so it can be created properly in a race-free manner.
See the thread at:
https://lore.kernel.org/r/20190731124349.4474-2-gregkh@linuxfoundation.org
for the details on how to do that.
thanks,
greg k-h
^ permalink raw reply
* [PATCH] Add optional chip erase functionality to AT25 EEPROM driver.
From: Joe Burmeister @ 2019-08-09 12:53 UTC (permalink / raw)
To: joe.burmeister, Rob Herring, Mark Rutland, Arnd Bergmann,
Greg Kroah-Hartman, Srinivas Kandagatla, YueHaibing,
Bartosz Golaszewski, devicetree, linux-kernel
Many, though not all, AT25s have an instruction for chip erase.
If there is one in the datasheet, it can be added to device tree.
Erase can then be done in userspace via the sysfs API with a new
"erase" device attribute. This matches the eeprom_93xx46 driver's
"erase".
Signed-off-by: Joe Burmeister <joe.burmeister@devtank.co.uk>
---
.../devicetree/bindings/eeprom/at25.txt | 2 +
drivers/misc/eeprom/at25.c | 83 ++++++++++++++++++-
2 files changed, 82 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/eeprom/at25.txt b/Documentation/devicetree/bindings/eeprom/at25.txt
index b3bde97dc199..c65d11e14c7a 100644
--- a/Documentation/devicetree/bindings/eeprom/at25.txt
+++ b/Documentation/devicetree/bindings/eeprom/at25.txt
@@ -19,6 +19,7 @@ Optional properties:
- spi-cpha : SPI shifted clock phase, as per spi-bus bindings.
- spi-cpol : SPI inverse clock polarity, as per spi-bus bindings.
- read-only : this parameter-less property disables writes to the eeprom
+- chip_erase_instruction : Chip erase instruction for this AT25, often 0xc7 or 0x62.
Obsolete legacy properties can be used in place of "size", "pagesize",
"address-width", and "read-only":
@@ -39,4 +40,5 @@ Example:
pagesize = <64>;
size = <32768>;
address-width = <16>;
+ chip_erase_instruction = <0x62>;
};
diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
index 99de6939cd5a..28141bc4028f 100644
--- a/drivers/misc/eeprom/at25.c
+++ b/drivers/misc/eeprom/at25.c
@@ -35,6 +35,7 @@ struct at25_data {
unsigned addrlen;
struct nvmem_config nvmem_config;
struct nvmem_device *nvmem;
+ u8 erase_instr;
};
#define AT25_WREN 0x06 /* latch the write enable */
@@ -59,6 +60,8 @@ struct at25_data {
*/
#define EE_TIMEOUT 25
+#define ERASE_TIMEOUT 2020
+
/*-------------------------------------------------------------------------*/
#define io_limit PAGE_SIZE /* bytes */
@@ -304,6 +307,71 @@ static int at25_fw_to_chip(struct device *dev, struct spi_eeprom *chip)
return 0;
}
+static void _eeprom_at25_store_erase_locked(struct at25_data *at25)
+{
+ unsigned long timeout, retries;
+ int sr, status;
+ u8 cp;
+
+ cp = AT25_WREN;
+ status = spi_write(at25->spi, &cp, 1);
+ if (status < 0) {
+ dev_dbg(&at25->spi->dev, "ERASE WREN --> %d\n", status);
+ return;
+ }
+ cp = at25->erase_instr;
+ status = spi_write(at25->spi, &cp, 1);
+ if (status < 0) {
+ dev_dbg(&at25->spi->dev, "CHIP_ERASE --> %d\n", status);
+ return;
+ }
+ /* Wait for non-busy status */
+ timeout = jiffies + msecs_to_jiffies(ERASE_TIMEOUT);
+ retries = 0;
+ do {
+ sr = spi_w8r8(at25->spi, AT25_RDSR);
+ if (sr < 0 || (sr & AT25_SR_nRDY)) {
+ dev_dbg(&at25->spi->dev,
+ "rdsr --> %d (%02x)\n", sr, sr);
+ /* at HZ=100, this is sloooow */
+ msleep(1);
+ continue;
+ }
+ if (!(sr & AT25_SR_nRDY))
+ return;
+ } while (retries++ < 200 || time_before_eq(jiffies, timeout));
+
+ if ((sr < 0) || (sr & AT25_SR_nRDY)) {
+ dev_err(&at25->spi->dev,
+ "chip erase, timeout after %u msecs\n",
+ jiffies_to_msecs(jiffies -
+ (timeout - ERASE_TIMEOUT)));
+ status = -ETIMEDOUT;
+ return;
+ }
+}
+
+
+static ssize_t eeprom_at25_store_erase(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct at25_data *at25 = dev_get_drvdata(dev);
+ int erase = 0;
+
+ sscanf(buf, "%d", &erase);
+ if (erase) {
+ mutex_lock(&at25->lock);
+ _eeprom_at25_store_erase_locked(at25);
+ mutex_unlock(&at25->lock);
+ }
+
+ return count;
+}
+
+static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_at25_store_erase);
+
+
static int at25_probe(struct spi_device *spi)
{
struct at25_data *at25 = NULL;
@@ -311,6 +379,7 @@ static int at25_probe(struct spi_device *spi)
int err;
int sr;
int addrlen;
+ int has_erase;
/* Chip description */
if (!spi->dev.platform_data) {
@@ -352,6 +421,9 @@ static int at25_probe(struct spi_device *spi)
spi_set_drvdata(spi, at25);
at25->addrlen = addrlen;
+ /* Optional chip erase instruction */
+ device_property_read_u8(&spi->dev, "chip_erase_instruction", &at25->erase_instr);
+
at25->nvmem_config.name = dev_name(&spi->dev);
at25->nvmem_config.dev = &spi->dev;
at25->nvmem_config.read_only = chip.flags & EE_READONLY;
@@ -370,17 +442,22 @@ static int at25_probe(struct spi_device *spi)
if (IS_ERR(at25->nvmem))
return PTR_ERR(at25->nvmem);
- dev_info(&spi->dev, "%d %s %s eeprom%s, pagesize %u\n",
+ has_erase = (!(chip.flags & EE_READONLY) && at25->erase_instr);
+
+ dev_info(&spi->dev, "%d %s %s eeprom%s, pagesize %u%s\n",
(chip.byte_len < 1024) ? chip.byte_len : (chip.byte_len / 1024),
(chip.byte_len < 1024) ? "Byte" : "KByte",
at25->chip.name,
(chip.flags & EE_READONLY) ? " (readonly)" : "",
- at25->chip.page_size);
+ at25->chip.page_size, (has_erase)?" <has erase>":"");
+
+ if (has_erase && device_create_file(&spi->dev, &dev_attr_erase))
+ dev_err(&spi->dev, "can't create erase interface\n");
+
return 0;
}
/*-------------------------------------------------------------------------*/
-
static const struct of_device_id at25_of_match[] = {
{ .compatible = "atmel,at25", },
{ }
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v8 05/21] clk: tegra: pll: Save and restore pll context
From: Dmitry Osipenko @ 2019-08-09 12:46 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree, rjw, viresh.kumar, linux-pm
In-Reply-To: <1565308020-31952-6-git-send-email-skomatineni@nvidia.com>
09.08.2019 2:46, Sowjanya Komatineni пишет:
> This patch implements save and restore of PLL context.
>
> During system suspend, core power goes off and looses the settings
> of the Tegra CAR controller registers.
>
> So during suspend entry pll context is stored and on resume it is
> restored back along with its state.
>
> Acked-by: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
> drivers/clk/tegra/clk-pll.c | 88 ++++++++++++++++++++++++++++-----------------
> drivers/clk/tegra/clk.h | 2 ++
> 2 files changed, 58 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
> index 1583f5fc992f..e52add2bbdbb 100644
> --- a/drivers/clk/tegra/clk-pll.c
> +++ b/drivers/clk/tegra/clk-pll.c
> @@ -1008,6 +1008,28 @@ static unsigned long clk_plle_recalc_rate(struct clk_hw *hw,
> return rate;
> }
>
> +static void tegra_clk_pll_restore_context(struct clk_hw *hw)
> +{
> + struct tegra_clk_pll *pll = to_clk_pll(hw);
> + struct clk_hw *parent = clk_hw_get_parent(hw);
> + unsigned long parent_rate = clk_hw_get_rate(parent);
> + unsigned long rate = clk_hw_get_rate(hw);
> + u32 val;
> +
> + if (clk_pll_is_enabled(hw))
> + return;
> +
> + if (pll->params->set_defaults)
> + pll->params->set_defaults(pll);
> +
> + clk_pll_set_rate(hw, rate, parent_rate);
> +
> + if (!__clk_get_enable_count(hw->clk))
> + clk_pll_disable(hw);
> + else
> + clk_pll_enable(hw);
> +}
drivers/clk/tegra/clk-pll.c: In function ‘tegra_clk_pll_restore_context’:
drivers/clk/tegra/clk-pll.c:1024:6: warning: unused variable ‘val’ [-Wunused-variable]
1024 | u32 val;
^ permalink raw reply
* Re: [PATCH v2 00/15] net: phy: adin: add support for Analog Devices PHYs
From: Ardelean, Alexandru @ 2019-08-09 12:32 UTC (permalink / raw)
To: davem@davemloft.net
Cc: andrew@lunn.ch, hkallweit1@gmail.com, devicetree@vger.kernel.org,
mark.rutland@arm.com, linux-kernel@vger.kernel.org,
f.fainelli@gmail.com, netdev@vger.kernel.org, robh+dt@kernel.org
In-Reply-To: <20190808.112431.1358324079415442430.davem@davemloft.net>
On Thu, 2019-08-08 at 11:24 -0700, David Miller wrote:
> [External]
>
> From: Alexandru Ardelean <alexandru.ardelean@analog.com>
> Date: Thu, 8 Aug 2019 15:30:11 +0300
>
> > This changeset adds support for Analog Devices Industrial Ethernet PHYs.
> > Particularly the PHYs this driver adds support for:
> > * ADIN1200 - Robust, Industrial, Low Power 10/100 Ethernet PHY
> > * ADIN1300 - Robust, Industrial, Low Latency 10/100/1000 Gigabit
> > Ethernet PHY
> >
> > The 2 chips are pin & register compatible with one another. The main
> > difference being that ADIN1200 doesn't operate in gigabit mode.
> >
> > The chips can be operated by the Generic PHY driver as well via the
> > standard IEEE PHY registers (0x0000 - 0x000F) which are supported by the
> > kernel as well. This assumes that configuration of the PHY has been done
> > completely in HW, according to spec, i.e. no extra SW configuration
> > required.
> >
> > This changeset also implements the ability to configure the chips via SW
> > registers.
> >
> > Datasheets:
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ADIN1300.pdf
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ADIN1200.pdf
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>
> I think, at a minimum, the c22 vs. c45 issues need to be discussed more
> and even if no code changes occur there is definitely some adjustments
> and clairifications that need to occur on this issue in the commit
> messages and/or documentation.
I guess I'll drop/defer some of the C45 stuff for now.
I don't know how decisions were done when the chips were created.
I am told that C45 works, but I may need to find out more on my end, since I am also new to/unclear on some items.
[My personal feeling about this]
I think there are some confusions [internally on our side] about what C45 is and how it should be done.
I guess it's part of developing knowledge/skills for developing PHYs as a company.
There's plenty of knowledge for how to do the electrical, low-power-stuff, etc, and even the datasheet sometimes feels
like it's for an ADC/DAC.
[My personal feeling about this]
Thanks
Alex
^ permalink raw reply
* Applied "regulator: qcom-rpmh: Sort the compatibles" to the regulator tree
From: Mark Brown @ 2019-08-09 12:31 UTC (permalink / raw)
To: Vinod Koul
Cc: Andy Gross, Bjorn Andersson, devicetree, Liam Girdwood,
linux-arm-msm, linux-kernel, Mark Brown, Mark Rutland,
Rob Herring
In-Reply-To: <20190809073616.1235-2-vkoul@kernel.org>
The patch
regulator: qcom-rpmh: Sort the compatibles
has been applied to the regulator tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-5.4
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
>From 7172fb7f3abea4787ca01dda7297241c4b0f0af5 Mon Sep 17 00:00:00 2001
From: Vinod Koul <vkoul@kernel.org>
Date: Fri, 9 Aug 2019 13:06:14 +0530
Subject: [PATCH] regulator: qcom-rpmh: Sort the compatibles
It helps to keep sorted order for compatibles, so sort them
Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
Link: https://lore.kernel.org/r/20190809073616.1235-2-vkoul@kernel.org
Signed-off-by: Mark Brown <broonie@kernel.org>
---
drivers/regulator/qcom-rpmh-regulator.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
index 693ffec62f3e..0ef2716da3bd 100644
--- a/drivers/regulator/qcom-rpmh-regulator.c
+++ b/drivers/regulator/qcom-rpmh-regulator.c
@@ -878,18 +878,14 @@ static int rpmh_regulator_probe(struct platform_device *pdev)
}
static const struct of_device_id rpmh_regulator_match_table[] = {
- {
- .compatible = "qcom,pm8998-rpmh-regulators",
- .data = pm8998_vreg_data,
- },
- {
- .compatible = "qcom,pmi8998-rpmh-regulators",
- .data = pmi8998_vreg_data,
- },
{
.compatible = "qcom,pm8005-rpmh-regulators",
.data = pm8005_vreg_data,
},
+ {
+ .compatible = "qcom,pm8009-rpmh-regulators",
+ .data = pm8009_vreg_data,
+ },
{
.compatible = "qcom,pm8150-rpmh-regulators",
.data = pm8150_vreg_data,
@@ -899,8 +895,12 @@ static const struct of_device_id rpmh_regulator_match_table[] = {
.data = pm8150l_vreg_data,
},
{
- .compatible = "qcom,pm8009-rpmh-regulators",
- .data = pm8009_vreg_data,
+ .compatible = "qcom,pm8998-rpmh-regulators",
+ .data = pm8998_vreg_data,
+ },
+ {
+ .compatible = "qcom,pmi8998-rpmh-regulators",
+ .data = pmi8998_vreg_data,
},
{}
};
--
2.20.1
^ permalink raw reply related
* Applied "regulator: dt-bindings: Sort the compatibles and nodes" to the regulator tree
From: Mark Brown @ 2019-08-09 12:31 UTC (permalink / raw)
To: Vinod Koul
Cc: Andy Gross, Bjorn Andersson, devicetree, Liam Girdwood,
linux-arm-msm, linux-kernel, Mark Brown, Mark Rutland,
Rob Herring
In-Reply-To: <20190809073616.1235-1-vkoul@kernel.org>
The patch
regulator: dt-bindings: Sort the compatibles and nodes
has been applied to the regulator tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-5.4
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
>From c6e20fa49818381dfa7288fad4c33b84408aab54 Mon Sep 17 00:00:00 2001
From: Vinod Koul <vkoul@kernel.org>
Date: Fri, 9 Aug 2019 13:06:13 +0530
Subject: [PATCH] regulator: dt-bindings: Sort the compatibles and nodes
It helps to keep sorted order for compatibles and nodes, so sort them
Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
Link: https://lore.kernel.org/r/20190809073616.1235-1-vkoul@kernel.org
Signed-off-by: Mark Brown <broonie@kernel.org>
---
.../regulator/qcom,rpmh-regulator.txt | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt b/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt
index 1a9cab50503a..bab9f71140b8 100644
--- a/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt
@@ -22,12 +22,12 @@ RPMh resource.
The names used for regulator nodes must match those supported by a given PMIC.
Supported regulator node names:
- PM8998: smps1 - smps13, ldo1 - ldo28, lvs1 - lvs2
- PMI8998: bob
PM8005: smps1 - smps4
+ PM8009: smps1 - smps2, ldo1 - ldo7
PM8150: smps1 - smps10, ldo1 - ldo18
PM8150L: smps1 - smps8, ldo1 - ldo11, bob, flash, rgb
- PM8009: smps1 - smps2, ld01 - ldo7
+ PM8998: smps1 - smps13, ldo1 - ldo28, lvs1 - lvs2
+ PMI8998: bob
========================
First Level Nodes - PMIC
@@ -36,12 +36,13 @@ First Level Nodes - PMIC
- compatible
Usage: required
Value type: <string>
- Definition: Must be one of: "qcom,pm8998-rpmh-regulators",
- "qcom,pmi8998-rpmh-regulators" or
- "qcom,pm8005-rpmh-regulators" or
- "qcom,pm8150-rpmh-regulators" or
- "qcom,pm8150l-rpmh-regulators" or
- "qcom,pm8009-rpmh-regulators".
+ Definition: Must be one of below:
+ "qcom,pm8005-rpmh-regulators"
+ "qcom,pm8009-rpmh-regulators"
+ "qcom,pm8150-rpmh-regulators"
+ "qcom,pm8150l-rpmh-regulators"
+ "qcom,pm8998-rpmh-regulators"
+ "qcom,pmi8998-rpmh-regulators"
- qcom,pmic-id
Usage: required
--
2.20.1
^ permalink raw reply related
* Applied "regulator: qcom-rpmh: Fix pmic5_bob voltage count" to the regulator tree
From: Mark Brown @ 2019-08-09 12:31 UTC (permalink / raw)
To: Vinod Koul
Cc: Andy Gross, Bjorn Andersson, devicetree, Liam Girdwood,
linux-arm-msm, linux-kernel, Mark Brown, Mark Rutland,
Rob Herring
In-Reply-To: <20190809073616.1235-3-vkoul@kernel.org>
The patch
regulator: qcom-rpmh: Fix pmic5_bob voltage count
has been applied to the regulator tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-5.4
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
>From 553c452d6093d66e7617ed6c68cc93547d07075f Mon Sep 17 00:00:00 2001
From: Vinod Koul <vkoul@kernel.org>
Date: Fri, 9 Aug 2019 13:06:15 +0530
Subject: [PATCH] regulator: qcom-rpmh: Fix pmic5_bob voltage count
pmic5_bob voltages count is 136 [0,135] so update it
Signed-off-by: Vinod Koul <vkoul@kernel.org>
Link: https://lore.kernel.org/r/20190809073616.1235-3-vkoul@kernel.org
Signed-off-by: Mark Brown <broonie@kernel.org>
---
drivers/regulator/qcom-rpmh-regulator.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
index 0ef2716da3bd..391ed844a251 100644
--- a/drivers/regulator/qcom-rpmh-regulator.c
+++ b/drivers/regulator/qcom-rpmh-regulator.c
@@ -698,7 +698,7 @@ static const struct rpmh_vreg_hw_data pmic5_bob = {
.regulator_type = VRM,
.ops = &rpmh_regulator_vrm_bypass_ops,
.voltage_range = REGULATOR_LINEAR_RANGE(300000, 0, 135, 32000),
- .n_voltages = 135,
+ .n_voltages = 136,
.pmic_mode_map = pmic_mode_map_pmic4_bob,
.of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
};
--
2.20.1
^ permalink raw reply related
* Applied "regulator: qcom-rpmh: Update PMIC modes for PMIC5" to the regulator tree
From: Mark Brown @ 2019-08-09 12:31 UTC (permalink / raw)
To: Vinod Koul
Cc: Andy Gross, Bjorn Andersson, devicetree, Liam Girdwood,
linux-arm-msm, linux-kernel, Mark Brown, Mark Rutland,
Rob Herring
In-Reply-To: <20190809073616.1235-4-vkoul@kernel.org>
The patch
regulator: qcom-rpmh: Update PMIC modes for PMIC5
has been applied to the regulator tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-5.4
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
>From 610f29e5cc0e8d5864dd049b0b3576d9437ae7b4 Mon Sep 17 00:00:00 2001
From: Vinod Koul <vkoul@kernel.org>
Date: Fri, 9 Aug 2019 13:06:16 +0530
Subject: [PATCH] regulator: qcom-rpmh: Update PMIC modes for PMIC5
Add the PMIC5 modes and use them pmic5 ldo and smps
Signed-off-by: Vinod Koul <vkoul@kernel.org>
Link: https://lore.kernel.org/r/20190809073616.1235-4-vkoul@kernel.org
Signed-off-by: Mark Brown <broonie@kernel.org>
---
drivers/regulator/qcom-rpmh-regulator.c | 52 +++++++++++++++++++++----
1 file changed, 45 insertions(+), 7 deletions(-)
diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
index 391ed844a251..db6c085da65e 100644
--- a/drivers/regulator/qcom-rpmh-regulator.c
+++ b/drivers/regulator/qcom-rpmh-regulator.c
@@ -50,6 +50,20 @@ enum rpmh_regulator_type {
#define PMIC4_BOB_MODE_AUTO 2
#define PMIC4_BOB_MODE_PWM 3
+#define PMIC5_LDO_MODE_RETENTION 3
+#define PMIC5_LDO_MODE_LPM 4
+#define PMIC5_LDO_MODE_HPM 7
+
+#define PMIC5_SMPS_MODE_RETENTION 3
+#define PMIC5_SMPS_MODE_PFM 4
+#define PMIC5_SMPS_MODE_AUTO 6
+#define PMIC5_SMPS_MODE_PWM 7
+
+#define PMIC5_BOB_MODE_PASS 2
+#define PMIC5_BOB_MODE_PFM 4
+#define PMIC5_BOB_MODE_AUTO 6
+#define PMIC5_BOB_MODE_PWM 7
+
/**
* struct rpmh_vreg_hw_data - RPMh regulator hardware configurations
* @regulator_type: RPMh accelerator type used to manage this
@@ -488,6 +502,14 @@ static const int pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
[REGULATOR_MODE_FAST] = -EINVAL,
};
+static const int pmic_mode_map_pmic5_ldo[REGULATOR_MODE_STANDBY + 1] = {
+ [REGULATOR_MODE_INVALID] = -EINVAL,
+ [REGULATOR_MODE_STANDBY] = PMIC5_LDO_MODE_RETENTION,
+ [REGULATOR_MODE_IDLE] = PMIC5_LDO_MODE_LPM,
+ [REGULATOR_MODE_NORMAL] = PMIC5_LDO_MODE_HPM,
+ [REGULATOR_MODE_FAST] = -EINVAL,
+};
+
static unsigned int rpmh_regulator_pmic4_ldo_of_map_mode(unsigned int rpmh_mode)
{
unsigned int mode;
@@ -518,6 +540,14 @@ static const int pmic_mode_map_pmic4_smps[REGULATOR_MODE_STANDBY + 1] = {
[REGULATOR_MODE_FAST] = PMIC4_SMPS_MODE_PWM,
};
+static const int pmic_mode_map_pmic5_smps[REGULATOR_MODE_STANDBY + 1] = {
+ [REGULATOR_MODE_INVALID] = -EINVAL,
+ [REGULATOR_MODE_STANDBY] = PMIC5_SMPS_MODE_RETENTION,
+ [REGULATOR_MODE_IDLE] = PMIC5_SMPS_MODE_PFM,
+ [REGULATOR_MODE_NORMAL] = PMIC5_SMPS_MODE_AUTO,
+ [REGULATOR_MODE_FAST] = PMIC5_SMPS_MODE_PWM,
+};
+
static unsigned int
rpmh_regulator_pmic4_smps_of_map_mode(unsigned int rpmh_mode)
{
@@ -552,6 +582,14 @@ static const int pmic_mode_map_pmic4_bob[REGULATOR_MODE_STANDBY + 1] = {
[REGULATOR_MODE_FAST] = PMIC4_BOB_MODE_PWM,
};
+static const int pmic_mode_map_pmic5_bob[REGULATOR_MODE_STANDBY + 1] = {
+ [REGULATOR_MODE_INVALID] = -EINVAL,
+ [REGULATOR_MODE_STANDBY] = -EINVAL,
+ [REGULATOR_MODE_IDLE] = PMIC5_BOB_MODE_PFM,
+ [REGULATOR_MODE_NORMAL] = PMIC5_BOB_MODE_AUTO,
+ [REGULATOR_MODE_FAST] = PMIC5_BOB_MODE_PWM,
+};
+
static unsigned int rpmh_regulator_pmic4_bob_of_map_mode(unsigned int rpmh_mode)
{
unsigned int mode;
@@ -643,7 +681,7 @@ static const struct rpmh_vreg_hw_data pmic5_pldo = {
.voltage_range = REGULATOR_LINEAR_RANGE(1504000, 0, 255, 8000),
.n_voltages = 256,
.hpm_min_load_uA = 10000,
- .pmic_mode_map = pmic_mode_map_pmic4_ldo,
+ .pmic_mode_map = pmic_mode_map_pmic5_ldo,
.of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
};
@@ -653,7 +691,7 @@ static const struct rpmh_vreg_hw_data pmic5_pldo_lv = {
.voltage_range = REGULATOR_LINEAR_RANGE(1504000, 0, 62, 8000),
.n_voltages = 63,
.hpm_min_load_uA = 10000,
- .pmic_mode_map = pmic_mode_map_pmic4_ldo,
+ .pmic_mode_map = pmic_mode_map_pmic5_ldo,
.of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
};
@@ -663,7 +701,7 @@ static const struct rpmh_vreg_hw_data pmic5_nldo = {
.voltage_range = REGULATOR_LINEAR_RANGE(320000, 0, 123, 8000),
.n_voltages = 124,
.hpm_min_load_uA = 30000,
- .pmic_mode_map = pmic_mode_map_pmic4_ldo,
+ .pmic_mode_map = pmic_mode_map_pmic5_ldo,
.of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
};
@@ -672,7 +710,7 @@ static const struct rpmh_vreg_hw_data pmic5_hfsmps510 = {
.ops = &rpmh_regulator_vrm_ops,
.voltage_range = REGULATOR_LINEAR_RANGE(320000, 0, 215, 8000),
.n_voltages = 216,
- .pmic_mode_map = pmic_mode_map_pmic4_smps,
+ .pmic_mode_map = pmic_mode_map_pmic5_smps,
.of_map_mode = rpmh_regulator_pmic4_smps_of_map_mode,
};
@@ -681,7 +719,7 @@ static const struct rpmh_vreg_hw_data pmic5_ftsmps510 = {
.ops = &rpmh_regulator_vrm_ops,
.voltage_range = REGULATOR_LINEAR_RANGE(300000, 0, 263, 4000),
.n_voltages = 264,
- .pmic_mode_map = pmic_mode_map_pmic4_smps,
+ .pmic_mode_map = pmic_mode_map_pmic5_smps,
.of_map_mode = rpmh_regulator_pmic4_smps_of_map_mode,
};
@@ -690,7 +728,7 @@ static const struct rpmh_vreg_hw_data pmic5_hfsmps515 = {
.ops = &rpmh_regulator_vrm_ops,
.voltage_range = REGULATOR_LINEAR_RANGE(2800000, 0, 4, 1600),
.n_voltages = 5,
- .pmic_mode_map = pmic_mode_map_pmic4_smps,
+ .pmic_mode_map = pmic_mode_map_pmic5_smps,
.of_map_mode = rpmh_regulator_pmic4_smps_of_map_mode,
};
@@ -699,7 +737,7 @@ static const struct rpmh_vreg_hw_data pmic5_bob = {
.ops = &rpmh_regulator_vrm_bypass_ops,
.voltage_range = REGULATOR_LINEAR_RANGE(300000, 0, 135, 32000),
.n_voltages = 136,
- .pmic_mode_map = pmic_mode_map_pmic4_bob,
+ .pmic_mode_map = pmic_mode_map_pmic5_bob,
.of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
};
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v8 11/21] clk: tegra: clk-dfll: Add suspend and resume support
From: Dmitry Osipenko @ 2019-08-09 12:23 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree, rjw, viresh.kumar, linux-pm
In-Reply-To: <1565308020-31952-12-git-send-email-skomatineni@nvidia.com>
09.08.2019 2:46, Sowjanya Komatineni пишет:
> This patch implements DFLL suspend and resume operation.
>
> During system suspend entry, CPU clock will switch CPU to safe
> clock source of PLLP and disables DFLL clock output.
>
> DFLL driver suspend confirms DFLL disable state and errors out on
> being active.
>
> DFLL is re-initialized during the DFLL driver resume as it goes
> through complete reset during suspend entry.
>
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
> drivers/clk/tegra/clk-dfll.c | 56 ++++++++++++++++++++++++++++++
> drivers/clk/tegra/clk-dfll.h | 2 ++
> drivers/clk/tegra/clk-tegra124-dfll-fcpu.c | 1 +
> 3 files changed, 59 insertions(+)
>
> diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
> index f8688c2ddf1a..eb298a5d7be9 100644
> --- a/drivers/clk/tegra/clk-dfll.c
> +++ b/drivers/clk/tegra/clk-dfll.c
> @@ -1487,6 +1487,7 @@ static int dfll_init(struct tegra_dfll *td)
> td->last_unrounded_rate = 0;
>
> pm_runtime_enable(td->dev);
> + pm_runtime_irq_safe(td->dev);
> pm_runtime_get_sync(td->dev);
>
> dfll_set_mode(td, DFLL_DISABLED);
> @@ -1513,6 +1514,61 @@ static int dfll_init(struct tegra_dfll *td)
> return ret;
> }
>
> +/**
> + * tegra_dfll_suspend - check DFLL is disabled
> + * @dev: DFLL device *
> + *
> + * DFLL clock should be disabled by the CPUFreq driver. So, make
> + * sure it is disabled and disable all clocks needed by the DFLL.
> + */
> +int tegra_dfll_suspend(struct device *dev)
> +{
> + struct tegra_dfll *td = dev_get_drvdata(dev);
> +
> + if (dfll_is_running(td)) {
> + dev_err(td->dev, "dfll is enabled while shouldn't be\n");
> + return -EBUSY;
> + }
> +
> + reset_control_assert(td->dvco_rst);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(tegra_dfll_suspend);
> +
> +/**
> + * tegra_dfll_resume - reinitialize DFLL on resume
> + * @dev: DFLL instance
> + *
> + * DFLL is disabled and reset during suspend and resume.
> + * So, reinitialize the DFLL IP block back for use.
> + * DFLL clock is enabled later in closed loop mode by CPUFreq
> + * driver before switching its clock source to DFLL output.
> + */
> +int tegra_dfll_resume(struct device *dev)
> +{
> + struct tegra_dfll *td = dev_get_drvdata(dev);
> +
> + reset_control_deassert(td->dvco_rst);
This doesn't look right because I assume that DFLL resetting is
synchronous and thus clk should be enabled in order for reset to
propagate inside hardware.
> + pm_runtime_get_sync(td->dev);
Hence it will be better to remove the above reset_control_deassert() and
add here:
reset_control_reset(td->dvco_rst);
> + dfll_set_mode(td, DFLL_DISABLED);
> + dfll_set_default_params(td);
> +
> + if (td->soc->init_clock_trimmers)
> + td->soc->init_clock_trimmers();
> +
> + dfll_set_open_loop_config(td);
> +
> + dfll_init_out_if(td);
> +
> + pm_runtime_put_sync(td->dev);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(tegra_dfll_resume);
> +
> /*
> * DT data fetch
> */
> diff --git a/drivers/clk/tegra/clk-dfll.h b/drivers/clk/tegra/clk-dfll.h
> index 1b14ebe7268b..fb209eb5f365 100644
> --- a/drivers/clk/tegra/clk-dfll.h
> +++ b/drivers/clk/tegra/clk-dfll.h
> @@ -42,5 +42,7 @@ int tegra_dfll_register(struct platform_device *pdev,
> struct tegra_dfll_soc_data *tegra_dfll_unregister(struct platform_device *pdev);
> int tegra_dfll_runtime_suspend(struct device *dev);
> int tegra_dfll_runtime_resume(struct device *dev);
> +int tegra_dfll_suspend(struct device *dev);
> +int tegra_dfll_resume(struct device *dev);
>
> #endif /* __DRIVERS_CLK_TEGRA_CLK_DFLL_H */
> diff --git a/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c b/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
> index e84b6d52cbbd..2ac2679d696d 100644
> --- a/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
> +++ b/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
> @@ -631,6 +631,7 @@ static int tegra124_dfll_fcpu_remove(struct platform_device *pdev)
> static const struct dev_pm_ops tegra124_dfll_pm_ops = {
> SET_RUNTIME_PM_OPS(tegra_dfll_runtime_suspend,
> tegra_dfll_runtime_resume, NULL)
> + SET_SYSTEM_SLEEP_PM_OPS(tegra_dfll_suspend, tegra_dfll_resume)
> };
>
> static struct platform_driver tegra124_dfll_fcpu_driver = {
>
^ permalink raw reply
* Re: [PATCH v8 08/21] clk: tegra: periph: Add restore_context support
From: Dmitry Osipenko @ 2019-08-09 12:20 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree, rjw, viresh.kumar, linux-pm
In-Reply-To: <5a5f9fb9-9cdd-5d91-4b0e-9bdb95b2625e@gmail.com>
09.08.2019 14:55, Dmitry Osipenko пишет:
> 09.08.2019 2:46, Sowjanya Komatineni пишет:
>> This patch implements restore_context support for clk-periph and
>> clk-sdmmc-mux clock operations to restore clock parent and rates
>> on system resume.
>>
>> During system suspend, core power goes off and looses the context
>> of the Tegra clock controller registers.
>>
>> So on system resume, clocks parent and rate are restored back to
>> the context before suspend based on cached data.
>>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
>> drivers/clk/tegra/clk-periph.c | 18 ++++++++++++++++++
>> drivers/clk/tegra/clk-sdmmc-mux.c | 12 ++++++++++++
>> 2 files changed, 30 insertions(+)
>>
>> diff --git a/drivers/clk/tegra/clk-periph.c b/drivers/clk/tegra/clk-periph.c
>> index 58437da25156..c9d28cbadccc 100644
>> --- a/drivers/clk/tegra/clk-periph.c
>> +++ b/drivers/clk/tegra/clk-periph.c
>> @@ -3,6 +3,7 @@
>> * Copyright (c) 2012, NVIDIA CORPORATION. All rights reserved.
>> */
>>
>> +#include <linux/clk.h>
>> #include <linux/clk-provider.h>
>> #include <linux/export.h>
>> #include <linux/slab.h>
>> @@ -99,6 +100,20 @@ static void clk_periph_disable(struct clk_hw *hw)
>> gate_ops->disable(gate_hw);
>> }
>>
>> +static void clk_periph_restore_context(struct clk_hw *hw)
>> +{
>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>> + const struct clk_ops *div_ops = periph->div_ops;
>> + struct clk_hw *div_hw = &periph->divider.hw;
>> + struct clk_hw *parent = clk_hw_get_parent(hw);
>> + int parent_id = clk_hw_get_parent_index(hw, parent);
>> +
>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_DIV))
>> + div_ops->restore_context(div_hw);
>> +
>> + clk_periph_set_parent(hw, parent_id);
>> +}
>> +
>> const struct clk_ops tegra_clk_periph_ops = {
>> .get_parent = clk_periph_get_parent,
>> .set_parent = clk_periph_set_parent,
>> @@ -108,6 +123,7 @@ const struct clk_ops tegra_clk_periph_ops = {
>> .is_enabled = clk_periph_is_enabled,
>> .enable = clk_periph_enable,
>> .disable = clk_periph_disable,
>> + .restore_context = clk_periph_restore_context,
>> };
>>
>> static const struct clk_ops tegra_clk_periph_nodiv_ops = {
>> @@ -116,6 +132,7 @@ static const struct clk_ops tegra_clk_periph_nodiv_ops = {
>> .is_enabled = clk_periph_is_enabled,
>> .enable = clk_periph_enable,
>> .disable = clk_periph_disable,
>> + .restore_context = clk_periph_restore_context,
>> };
>>
>> static const struct clk_ops tegra_clk_periph_no_gate_ops = {
>> @@ -124,6 +141,7 @@ static const struct clk_ops tegra_clk_periph_no_gate_ops = {
>> .recalc_rate = clk_periph_recalc_rate,
>> .round_rate = clk_periph_round_rate,
>> .set_rate = clk_periph_set_rate,
>> + .restore_context = clk_periph_restore_context,
>> };
>>
>> static struct clk *_tegra_clk_register_periph(const char *name,
>> diff --git a/drivers/clk/tegra/clk-sdmmc-mux.c b/drivers/clk/tegra/clk-sdmmc-mux.c
>> index a5cd3e31dbae..8db48966b100 100644
>> --- a/drivers/clk/tegra/clk-sdmmc-mux.c
>> +++ b/drivers/clk/tegra/clk-sdmmc-mux.c
>> @@ -194,6 +194,17 @@ static void clk_sdmmc_mux_disable(struct clk_hw *hw)
>> gate_ops->disable(gate_hw);
>> }
>>
>> +static void clk_sdmmc_mux_restore_context(struct clk_hw *hw)
>> +{
>> + struct clk_hw *parent = clk_hw_get_parent(hw);
>> + unsigned long parent_rate = clk_hw_get_rate(parent);
>> + unsigned long rate = clk_hw_get_rate(hw);
>> + int parent_id = clk_hw_get_parent_index(hw, parent);
>> +
>> + clk_sdmmc_mux_set_parent(hw, parent_id);
>> + clk_sdmmc_mux_set_rate(hw, rate, parent_rate);
>
> For the periph clocks you're restoring rate at first and then the
> parent, while here it's the other way around. I'm wondering if there is
> any difference in practice and thus whether rate's divider need to be
> set to a some safe value before switching to a new parent that has a
> higher clock rate than the old parent.
Although, I guess that all peripheral clocks should be disabled at this
point of resume. Correct?
>> +}
>> +
>> static const struct clk_ops tegra_clk_sdmmc_mux_ops = {
>> .get_parent = clk_sdmmc_mux_get_parent,
>> .set_parent = clk_sdmmc_mux_set_parent,
>> @@ -203,6 +214,7 @@ static const struct clk_ops tegra_clk_sdmmc_mux_ops = {
>> .is_enabled = clk_sdmmc_mux_is_enabled,
>> .enable = clk_sdmmc_mux_enable,
>> .disable = clk_sdmmc_mux_disable,
>> + .restore_context = clk_sdmmc_mux_restore_context,
>> };
>>
>> struct clk *tegra_clk_register_sdmmc_mux_div(const char *name,
>>
>
^ permalink raw reply
* [PATCH 2/3 v3] dt-bindings: mips: Add gardena vendor prefix and board description
From: Stefan Roese @ 2019-08-09 12:19 UTC (permalink / raw)
To: linux-mips; +Cc: Paul Burton, Rob Herring, devicetree
In-Reply-To: <20190809121918.25047-1-sr@denx.de>
This patch adds the vendor prefix for gardena and a short description
including the compatible string for the "GARDENA smart Gateway" based
on the MT7688 SoC.
Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Paul Burton <paul.burton@mips.com>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
---
v3:
- New patch
.../devicetree/bindings/mips/ralink/gardena.txt | 9 +++++++++
Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
2 files changed, 11 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mips/ralink/gardena.txt
diff --git a/Documentation/devicetree/bindings/mips/ralink/gardena.txt b/Documentation/devicetree/bindings/mips/ralink/gardena.txt
new file mode 100644
index 000000000000..4022fe61a8ee
--- /dev/null
+++ b/Documentation/devicetree/bindings/mips/ralink/gardena.txt
@@ -0,0 +1,9 @@
+GARDENA smart Gateway (MT7688)
+
+This board is based on the MediaTek MT7688 and equipped with 128 MiB
+of DDR and 8 MiB of flash (SPI NOR) and additional 128MiB SPI NAND
+storage.
+
+------------------------------
+Required root node properties:
+- compatible = "gardena,smartGatewayMT7688";
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 6992bbbbffab..73166adfd4ad 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -337,6 +337,8 @@ patternProperties:
description: Freescale Semiconductor
"^fujitsu,.*":
description: Fujitsu Ltd.
+ "^gardena,.*":
+ description: GARDENA GmbH
"^gateworks,.*":
description: Gateworks Corporation
"^gcw,.*":
--
2.22.0
^ permalink raw reply related
* Re: [PATCH 07/19] irqchip/mmp: mask off interrupts from other cores
From: Marc Zyngier @ 2019-08-09 12:18 UTC (permalink / raw)
To: Lubomir Rintel, Olof Johansson
Cc: Rob Herring, Mark Rutland, Thomas Gleixner, Jason Cooper,
Kishon Vijay Abraham I, Russell King, Michael Turquette,
Stephen Boyd, devicetree, linux-kernel, linux-arm-kernel,
linux-clk, Andres Salomon
In-Reply-To: <20190809093158.7969-8-lkundrak@v3.sk>
On 09/08/2019 10:31, Lubomir Rintel wrote:
> From: Andres Salomon <dilinger@queued.net>
>
> On mmp3, there's an extra set of ICU registers (ICU2) that handle
> interrupts on the extra cores. When masking off interrupts on MP1,
> these should be masked as well.
>
> We add a new interrupt controller via device tree to identify when we're
> looking at an mmp3 machine via compatible field of "marvell,mmp3-intc".
>
> [lkundrak@v3.sk: Changed "mrvl,mmp3-intc" compatible strings to
> "marvell,mmp3-intc". Tidied up the subject line a bit.]
>
> Signed-off-by: Andres Salomon <dilinger@queued.net>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
>
> ---
> arch/arm/mach-mmp/regs-icu.h | 3 +++
> drivers/irqchip/irq-mmp.c | 51 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 54 insertions(+)
>
> diff --git a/arch/arm/mach-mmp/regs-icu.h b/arch/arm/mach-mmp/regs-icu.h
> index 0375d5a7fcb2b..410743d2b4020 100644
> --- a/arch/arm/mach-mmp/regs-icu.h
> +++ b/arch/arm/mach-mmp/regs-icu.h
> @@ -11,6 +11,9 @@
> #define ICU_VIRT_BASE (AXI_VIRT_BASE + 0x82000)
> #define ICU_REG(x) (ICU_VIRT_BASE + (x))
>
> +#define ICU2_VIRT_BASE (AXI_VIRT_BASE + 0x84000)
> +#define ICU2_REG(x) (ICU2_VIRT_BASE + (x))
> +
> #define ICU_INT_CONF(n) ICU_REG((n) << 2)
> #define ICU_INT_CONF_MASK (0xf)
>
> diff --git a/drivers/irqchip/irq-mmp.c b/drivers/irqchip/irq-mmp.c
> index cd8d2253f56d1..25497c75cc861 100644
> --- a/drivers/irqchip/irq-mmp.c
> +++ b/drivers/irqchip/irq-mmp.c
> @@ -44,6 +44,7 @@ struct icu_chip_data {
> unsigned int conf_enable;
> unsigned int conf_disable;
> unsigned int conf_mask;
> + unsigned int conf2_mask;
> unsigned int clr_mfp_irq_base;
> unsigned int clr_mfp_hwirq;
> struct irq_domain *domain;
> @@ -53,9 +54,11 @@ struct mmp_intc_conf {
> unsigned int conf_enable;
> unsigned int conf_disable;
> unsigned int conf_mask;
> + unsigned int conf2_mask;
> };
>
> static void __iomem *mmp_icu_base;
> +static void __iomem *mmp_icu2_base;
> static struct icu_chip_data icu_data[MAX_ICU_NR];
> static int max_icu_nr;
>
> @@ -98,6 +101,16 @@ static void icu_mask_irq(struct irq_data *d)
> r &= ~data->conf_mask;
> r |= data->conf_disable;
> writel_relaxed(r, mmp_icu_base + (hwirq << 2));
> +
> + if (data->conf2_mask) {
> + /*
> + * ICU1 (above) only controls PJ4 MP1; if using SMP,
> + * we need to also mask the MP2 and MM cores via ICU2.
> + */
> + r = readl_relaxed(mmp_icu2_base + (hwirq << 2));
> + r &= ~data->conf2_mask;
> + writel_relaxed(r, mmp_icu2_base + (hwirq << 2));
> + }
> } else {
> r = readl_relaxed(data->reg_mask) | (1 << hwirq);
> writel_relaxed(r, data->reg_mask);
> @@ -201,6 +214,14 @@ static const struct mmp_intc_conf mmp2_conf = {
> MMP2_ICU_INT_ROUTE_PJ4_FIQ,
> };
>
> +static struct mmp_intc_conf mmp3_conf = {
> + .conf_enable = 0x20,
> + .conf_disable = 0x0,
> + .conf_mask = MMP2_ICU_INT_ROUTE_PJ4_IRQ |
> + MMP2_ICU_INT_ROUTE_PJ4_FIQ,
> + .conf2_mask = 0xf0,
> +};
> +
> static void __exception_irq_entry mmp_handle_irq(struct pt_regs *regs)
> {
> int hwirq;
> @@ -364,6 +385,14 @@ static int __init mmp_init_bases(struct device_node *node)
> pr_err("Failed to get interrupt controller register\n");
> return -ENOMEM;
> }
> + if (of_device_is_compatible(node, "marvell,mmp3-intc")) {
Instead of harcoding the compatible property once more, why don't you
simply pass a flag from mmpx_of_init()?
> + mmp_icu2_base = of_iomap(node, 1);
> + if (!mmp_icu2_base) {
> + pr_err("Failed to get interrupt controller register #2\n");
> + iounmap(mmp_icu_base);
> + return -ENOMEM;
> + }
> + }
>
> icu_data[0].virq_base = 0;
> icu_data[0].domain = irq_domain_add_linear(node, nr_irqs,
> @@ -386,6 +415,8 @@ static int __init mmp_init_bases(struct device_node *node)
> irq_dispose_mapping(icu_data[0].virq_base + i);
> }
> irq_domain_remove(icu_data[0].domain);
> + if (of_device_is_compatible(node, "marvell,mmp3-intc"))
> + iounmap(mmp_icu2_base);
> iounmap(mmp_icu_base);
> return -EINVAL;
> }
> @@ -428,6 +459,26 @@ static int __init mmp2_of_init(struct device_node *node,
> }
> IRQCHIP_DECLARE(mmp2_intc, "mrvl,mmp2-intc", mmp2_of_init);
>
> +static int __init mmp3_of_init(struct device_node *node,
> + struct device_node *parent)
> +{
> + int ret;
> +
> + ret = mmp_init_bases(node);
> + if (ret < 0)
> + return ret;
> +
> + icu_data[0].conf_enable = mmp3_conf.conf_enable;
> + icu_data[0].conf_disable = mmp3_conf.conf_disable;
> + icu_data[0].conf_mask = mmp3_conf.conf_mask;
> + icu_data[0].conf2_mask = mmp3_conf.conf2_mask;
> + irq_set_default_host(icu_data[0].domain);
Why do you need this? On a fully DT-ified platform, there should be no
notion of a default domain.
> + set_handle_irq(mmp2_handle_irq);
> + max_icu_nr = 1;
> + return 0;
> +}
> +IRQCHIP_DECLARE(mmp3_intc, "marvell,mmp3-intc", mmp3_of_init);
> +
> static int __init mmp2_mux_of_init(struct device_node *node,
> struct device_node *parent)
> {
>
Thanks,
M.
--
Jazz is not dead, it just smells funny...
^ permalink raw reply
* Re: [PATCH v8 10/21] clk: tegra: clk-super: Add restore-context support
From: Dmitry Osipenko @ 2019-08-09 12:17 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree, rjw, viresh.kumar, linux-pm
In-Reply-To: <1565308020-31952-11-git-send-email-skomatineni@nvidia.com>
09.08.2019 2:46, Sowjanya Komatineni пишет:
> This patch implements restore_context for clk_super_mux and clk_super.
>
> During system supend, core power goes off the and context of Tegra
> CAR registers is lost.
>
> So on system resume, context of super clock registers are restored
> to have them in same state as before suspend.
>
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
> drivers/clk/tegra/clk-super.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/drivers/clk/tegra/clk-super.c b/drivers/clk/tegra/clk-super.c
> index e2a1e95a8db7..74c9e913e41c 100644
> --- a/drivers/clk/tegra/clk-super.c
> +++ b/drivers/clk/tegra/clk-super.c
> @@ -124,9 +124,18 @@ static int clk_super_set_parent(struct clk_hw *hw, u8 index)
> return err;
> }
>
> +static void clk_super_mux_restore_context(struct clk_hw *hw)
> +{
> + struct clk_hw *parent = clk_hw_get_parent(hw);
> + int parent_id = clk_hw_get_parent_index(hw, parent);
> +
> + clk_super_set_parent(hw, parent_id);
All Super clocks have a divider, including the "MUX". Thus I'm wondering
if there is a chance that divider's configuration may differ on resume
from what it was on suspend.
> +}
> +
> static const struct clk_ops tegra_clk_super_mux_ops = {
> .get_parent = clk_super_get_parent,
> .set_parent = clk_super_set_parent,
> + .restore_context = clk_super_mux_restore_context,
> };
>
> static long clk_super_round_rate(struct clk_hw *hw, unsigned long rate,
> @@ -162,12 +171,24 @@ static int clk_super_set_rate(struct clk_hw *hw, unsigned long rate,
> return super->div_ops->set_rate(div_hw, rate, parent_rate);
> }
>
> +static void clk_super_restore_context(struct clk_hw *hw)
> +{
> + struct tegra_clk_super_mux *super = to_clk_super_mux(hw);
> + struct clk_hw *div_hw = &super->frac_div.hw;
> + struct clk_hw *parent = clk_hw_get_parent(hw);
> + int parent_id = clk_hw_get_parent_index(hw, parent);
> +
> + super->div_ops->restore_context(div_hw);
> + clk_super_set_parent(hw, parent_id);
> +}
> +
> const struct clk_ops tegra_clk_super_ops = {
> .get_parent = clk_super_get_parent,
> .set_parent = clk_super_set_parent,
> .set_rate = clk_super_set_rate,
> .round_rate = clk_super_round_rate,
> .recalc_rate = clk_super_recalc_rate,
> + .restore_context = clk_super_restore_context,
> };
>
> struct clk *tegra_clk_register_super_mux(const char *name,
>
^ permalink raw reply
* Re: [PATCH v6 03/13] media: v4l2-fwnode: add initial connector parsing support
From: Marco Felsch @ 2019-08-09 12:16 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Hans Verkuil, Mauro Carvalho Chehab, sakari.ailus, hans.verkuil,
jacopo+renesas, robh+dt, linux-media, devicetree, kernel,
Jacopo Mondi
In-Reply-To: <20190516165114.GP14820@pendragon.ideasonboard.com>
Hi Laurent,
On 19-05-16 19:51, Laurent Pinchart wrote:
> Hello Marco,
>
> Thank you for the patch.
Thanks for the review.
>
> On Tue, May 14, 2019 at 03:20:04PM -0300, Mauro Carvalho Chehab wrote:
> > Em Mon, 6 May 2019 12:10:41 +0200 Hans Verkuil escreveu:
> > > On 4/15/19 2:44 PM, Marco Felsch wrote:
> > > > The patch adds the initial connector parsing code, so we can move from a
> > > > driver specific parsing code to a generic one. Currently only the
> > > > generic fields and the analog-connector specific fields are parsed. Parsing
> > > > the other connector specific fields can be added by a simple callbacks.
> > > >
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > ---
> > > > [1] https://patchwork.kernel.org/cover/10794703/
> > > >
> > > > v6:
> > > > - use 'unsigned int' count var
> > > > - fix comment and style issues
> > > > - place '/* fall through */' to correct places
> > > > - fix error handling and cleanup by releasing fwnode
> > > > - drop vga and dvi parsing support as those connectors are rarely used
> > > > these days
> > > >
> > > > v5:
> > > > - s/strlcpy/strscpy/
> > > >
> > > > v2-v4:
> > > > - nothing since the patch was squashed from series [1] into this
> > > > series.
> > > >
> > > > drivers/media/v4l2-core/v4l2-fwnode.c | 111 ++++++++++++++++++++++++++
> > > > include/media/v4l2-fwnode.h | 16 ++++
> > > > 2 files changed, 127 insertions(+)
> > > >
> > > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > > index 20571846e636..f1cca95c8fef 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > > @@ -592,6 +592,117 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
> > > > }
> > > > EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> > > >
> > > > +static const struct v4l2_fwnode_connector_conv {
> > > > + enum v4l2_connector_type type;
> > > > + const char *name;
>
> Maybe compatible instead of name ?
Okay, I can change that.
> > > > +} connectors[] = {
> > > > + {
> > > > + .type = V4L2_CON_COMPOSITE,
> > > > + .name = "composite-video-connector",
> > > > + }, {
> > > > + .type = V4L2_CON_SVIDEO,
> > > > + .name = "svideo-connector",
> > > > + }, {
> > > > + .type = V4L2_CON_HDMI,
> > > > + .name = "hdmi-connector",
> > > > + },
> > > > +};
> > > > +
> > > > +static enum v4l2_connector_type
> > > > +v4l2_fwnode_string_to_connector_type(const char *con_str)
> > > > +{
> > > > + unsigned int i;
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(connectors); i++)
> > > > + if (!strcmp(con_str, connectors[i].name))
> > > > + return connectors[i].type;
> > > > +
> > > > + /* no valid connector found */
>
> The usual comment style in this file is to start with a capital letter
> and end sentences with a period. I would however drop this comment, it's
> not very useful. The other comments should be updated accordingly.
>
I will change my comments and drop this one.
> > > > + return V4L2_CON_UNKNOWN;
> > > > +}
> > > > +
> > > > +static int
> > > > +v4l2_fwnode_connector_parse_analog(struct fwnode_handle *fwnode,
> > > > + struct v4l2_fwnode_connector *vc)
> > > > +{
> > > > + u32 tvnorms;
> > > > + int ret;
> > > > +
> > > > + ret = fwnode_property_read_u32(fwnode, "tvnorms", &tvnorms);
> > > > +
> > > > + /* tvnorms is optional */
> > > > + vc->connector.analog.supported_tvnorms = ret ? V4L2_STD_ALL : tvnorms;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
>
> Please document all exported functions with kerneldoc.
It is documented within the header file. To be aligned with the other
functions I wouldn't change that.
> > > > +int v4l2_fwnode_parse_connector(struct fwnode_handle *__fwnode,
> > > > + struct v4l2_fwnode_connector *connector)
> > > > +{
> > > > + struct fwnode_handle *fwnode;
> > > > + struct fwnode_endpoint __ep;
> > > > + const char *c_type_str, *label;
> > > > + int ret;
> > > > +
> > > > + memset(connector, 0, sizeof(*connector));
> > > > +
> > > > + fwnode = fwnode_graph_get_remote_port_parent(__fwnode);
>
> I would rename the argument __fwnode to fwnode, and rename the fwnode
> variable to remote (or similar) to make this clearer.
Okay.
>
> > > > + if (!fwnode)
> > > > + return -EINVAL;
>
> Is EINVAL the right error here ? Wouldn't it be useful for the caller to
> differentiate between unconnected connector nodes and invalid ones ?
Yes it would. Should I return ENOLINK instead?
>
> > > > +
> > > > + /* parse all common properties first */
> > > > + /* connector-type is stored within the compatible string */
> > > > + ret = fwnode_property_read_string(fwnode, "compatible", &c_type_str);
>
> Prefixing or postfixing names with types is usually frowned upon. You
> could rename this to type_name for instance.
Okay.
> > > > + if (ret) {
> > > > + fwnode_handle_put(fwnode);
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + connector->type = v4l2_fwnode_string_to_connector_type(c_type_str);
> > > > +
> > > > + fwnode_graph_parse_endpoint(__fwnode, &__ep);
> > > > + connector->remote_port = __ep.port;
> > > > + connector->remote_id = __ep.id;
> > > > +
> > > > + ret = fwnode_property_read_string(fwnode, "label", &label);
> > > > + if (!ret) {
> > > > + /* ensure label doesn't exceed V4L2_CONNECTOR_MAX_LABEL size */
> > > > + strscpy(connector->label, label, V4L2_CONNECTOR_MAX_LABEL);
> > > > + } else {
> > > > + /*
> > > > + * labels are optional, if none is given create one:
> > > > + * <connector-type-string>@port<endpoint_port>/ep<endpoint_id>
> > > > + */
> > > > + snprintf(connector->label, V4L2_CONNECTOR_MAX_LABEL,
> > > > + "%s@port%u/ep%u", c_type_str, connector->remote_port,
> > > > + connector->remote_id);
>
> Should we really try to create labels when none is available ? If so
> this needs much more careful thoughts, we need to think about what the
> label will be used for, and create a good naming scheme accordingly. If
> the label will be displayed to the end-user I don't think the above name
> would be very useful, it would be best to leave it empty and let
> applications create a name based on the connector type and other
> information they have at their disposal.
Hm.. I don't have a strong opinion on that. If the others are with you I
will leave it empty.
> > > > + }
> > > > +
> > > > + /* now parse the connector specific properties */
> > > > + switch (connector->type) {
> > > > + case V4L2_CON_COMPOSITE:
> > > > + /* fall through */
>
> I don't think you need a fall-through comment when the two cases are
> adjacent with no line in-between.
Hm.. I don't know the compiler behaviour. According the official
gcc documentation [1] I would not leave that.
[1] https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
>
> > > > + case V4L2_CON_SVIDEO:
> > > > + ret = v4l2_fwnode_connector_parse_analog(fwnode, connector);
> > > > + break;
> > > > + case V4L2_CON_HDMI:
> > > > + pr_warn("Connector specific parsing is currently not supported for %s\n",
> > > > + c_type_str);
> > >
> > > Why warn? Just drop this.
> >
> > good point. I would prefer to have some warning here, in order to warn a
> > developer that might be using it that this part of the code would require
> > some change.
> >
> > perhaps pr_warn_once()?
> >
> > > > + ret = 0;
> > > > + break;
>
> If it's not supported we should warn and return an error. Otherwise we
> should be silent and return success. Combining a warning with success
> isn't a good idea, this is either a normal case or an error, not both.
The generic part still applies and is valid. That was the reason why I
did return success.
> > > > + case V4L2_CON_UNKNOWN:
> > > > + /* fall through */
> > > > + default:
> > > > + pr_err("Unknown connector type\n");
> > > > + ret = -EINVAL;
> > > > + };
> > > > +
> > > > + fwnode_handle_put(fwnode);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(v4l2_fwnode_parse_connector);
> > > > +
> > > > static int
> > > > v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
> > > > struct v4l2_async_notifier *notifier,
> > > > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> > > > index f4df1b95c5ef..e072f2915ddb 100644
> > > > --- a/include/media/v4l2-fwnode.h
> > > > +++ b/include/media/v4l2-fwnode.h
> > > > @@ -269,6 +269,22 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
> > > > */
> > > > void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
> > > >
>
> And I see here that the function is documented. One more reason to move
> kerneldoc to the .c files...
Please check my comment above.
> > > > +/**
> > > > + * v4l2_fwnode_parse_connector() - parse the connector on endpoint
> > > > + * @fwnode: pointer to the endpoint's fwnode handle where the connector is
> > > > + * connected to
>
> This is very unclear, I would interpret that as the remote endpoint, not
> the local endpoint. Could you please try to clarify the documentation ?
Hm.. I have no good idea how I should describe it..
"""
The device (local) endpoint fwnode handle on which the connector is
connected to using the remote-enpoint property.
"""
Regards,
Marco
> > > > + * @connector: pointer to the V4L2 fwnode connector data structure
> > > > + *
> > > > + * Fill the connector data structure with the connector type, label and the
> > > > + * endpoint id and port where the connector belongs to. If no label is present
> > > > + * a unique one will be created. Labels with more than 40 characters are cut.
> > > > + *
> > > > + * Return: %0 on success or a negative error code on failure:
> > > > + * %-EINVAL on parsing failure
> > > > + */
> > > > +int v4l2_fwnode_parse_connector(struct fwnode_handle *fwnode,
> > > > + struct v4l2_fwnode_connector *connector);
> > > > +
> > > > /**
> > > > * typedef parse_endpoint_func - Driver's callback function to be called on
> > > > * each V4L2 fwnode endpoint.
>
> --
> Regards,
>
> Laurent Pinchart
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH 05/19] irqchip/mmp: do not use of_address_to_resource() to get mux regs
From: Marc Zyngier @ 2019-08-09 12:12 UTC (permalink / raw)
To: Lubomir Rintel, Olof Johansson
Cc: Rob Herring, Mark Rutland, Thomas Gleixner, Jason Cooper,
Kishon Vijay Abraham I, Russell King, Michael Turquette,
Stephen Boyd, devicetree, linux-kernel, linux-arm-kernel,
linux-clk, Pavel Machek
In-Reply-To: <20190809093158.7969-6-lkundrak@v3.sk>
On 09/08/2019 10:31, Lubomir Rintel wrote:
> The "regs" property of the "mrvl,mmp2-mux-intc" devices are silly. They
> are offsets from intc's base, not addresses on the parent bus. At this
> point it probably can't be fixed.
>
> On an OLPC XO-1.75 machine, the muxes are children of the intc, not the
> axi bus, and thus of_address_to_resource() won't work. We should treat
> the values as mere integers as opposed to bus addresses.
>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Acked-by: Pavel Machek <pavel@ucw.cz>
>
> ---
> drivers/irqchip/irq-mmp.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/irqchip/irq-mmp.c b/drivers/irqchip/irq-mmp.c
> index 14618dc0bd396..af9cba4a51c2e 100644
> --- a/drivers/irqchip/irq-mmp.c
> +++ b/drivers/irqchip/irq-mmp.c
> @@ -424,9 +424,9 @@ IRQCHIP_DECLARE(mmp2_intc, "mrvl,mmp2-intc", mmp2_of_init);
> static int __init mmp2_mux_of_init(struct device_node *node,
> struct device_node *parent)
> {
> - struct resource res;
> int i, ret, irq, j = 0;
> u32 nr_irqs, mfp_irq;
> + u32 reg[4];
>
> if (!parent)
> return -ENODEV;
> @@ -438,18 +438,20 @@ static int __init mmp2_mux_of_init(struct device_node *node,
> pr_err("Not found mrvl,intc-nr-irqs property\n");
> return -EINVAL;
> }
> - ret = of_address_to_resource(node, 0, &res);
> +
> + /*
> + * For historical reasonsm, the "regs" property of the
> + * mrvl,mmp2-mux-intc is not a regular * "regs" property containing
> + * addresses on the parent bus, but offsets from the intc's base.
> + * That is why we can't use of_address_to_resource() here.
> + */
> + ret = of_property_read_u32_array(node, "reg", reg, ARRAY_SIZE(reg));
This will return 0 even if you've read less than your expected 4 u32s.
You may want to try of_property_read_variable_u32_array instead.
> if (ret < 0) {
> pr_err("Not found reg property\n");
> return -EINVAL;
> }
> - icu_data[i].reg_status = mmp_icu_base + res.start;
> - ret = of_address_to_resource(node, 1, &res);
> - if (ret < 0) {
> - pr_err("Not found reg property\n");
> - return -EINVAL;
> - }
> - icu_data[i].reg_mask = mmp_icu_base + res.start;
> + icu_data[i].reg_status = mmp_icu_base + reg[0];
> + icu_data[i].reg_mask = mmp_icu_base + reg[2];
> icu_data[i].cascade_irq = irq_of_parse_and_map(node, 0);
> if (!icu_data[i].cascade_irq)
> return -EINVAL;
>
Thanks,
M.
--
Jazz is not dead, it just smells funny...
^ permalink raw reply
* Re: [PATCH v8 09/21] clk: tegra: clk-super: Fix to enable PLLP branches to CPU
From: Dmitry Osipenko @ 2019-08-09 12:11 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree, rjw, viresh.kumar, linux-pm
In-Reply-To: <1565308020-31952-10-git-send-email-skomatineni@nvidia.com>
09.08.2019 2:46, Sowjanya Komatineni пишет:
> This patch has a fix to enable PLLP branches to CPU before changing
> the CPU cluster clock source to PLLP for Gen5 Super clock and
> disables PLLP branches to CPU when not in use.
>
> During system suspend entry and exit, CPU source will be switched
> to PLLP and this needs PLLP branches to be enabled to CPU prior to
> the switch.
>
> On system resume, warmboot code enables PLLP branches to CPU and
> powers up the CPU with PLLP clock source.
>
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
> drivers/clk/tegra/clk-super.c | 14 ++++++++++++++
> drivers/clk/tegra/clk-tegra-super-gen4.c | 7 ++++++-
> drivers/clk/tegra/clk.c | 14 ++++++++++++++
> drivers/clk/tegra/clk.h | 5 +++++
> 4 files changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/tegra/clk-super.c b/drivers/clk/tegra/clk-super.c
> index 39ef31b46df5..e2a1e95a8db7 100644
> --- a/drivers/clk/tegra/clk-super.c
> +++ b/drivers/clk/tegra/clk-super.c
> @@ -28,6 +28,9 @@
> #define super_state_to_src_shift(m, s) ((m->width * s))
> #define super_state_to_src_mask(m) (((1 << m->width) - 1))
>
> +#define CCLK_SRC_PLLP_OUT0 4
> +#define CCLK_SRC_PLLP_OUT4 5
> +
> static u8 clk_super_get_parent(struct clk_hw *hw)
> {
> struct tegra_clk_super_mux *mux = to_clk_super_mux(hw);
> @@ -97,12 +100,23 @@ static int clk_super_set_parent(struct clk_hw *hw, u8 index)
> if (index == mux->div2_index)
> index = mux->pllx_index;
> }
> +
> + /* enable PLLP branches to CPU before selecting PLLP source */
> + if ((mux->flags & TEGRA210_CPU_CLK) &&
> + (index == CCLK_SRC_PLLP_OUT0 || index == CCLK_SRC_PLLP_OUT4))
> + tegra_clk_set_pllp_out_cpu(true);
> +
> val &= ~((super_state_to_src_mask(mux)) << shift);
> val |= (index & (super_state_to_src_mask(mux))) << shift;
>
> writel_relaxed(val, mux->reg);
> udelay(2);
>
> + /* disable PLLP branches to CPU if not used */
> + if ((mux->flags & TEGRA210_CPU_CLK) &&
> + index != CCLK_SRC_PLLP_OUT0 && index != CCLK_SRC_PLLP_OUT4)
> + tegra_clk_set_pllp_out_cpu(false);
> +
> out:
> if (mux->lock)
> spin_unlock_irqrestore(mux->lock, flags);
> diff --git a/drivers/clk/tegra/clk-tegra-super-gen4.c b/drivers/clk/tegra/clk-tegra-super-gen4.c
> index cdfe7c9697e1..98538f79b0c4 100644
> --- a/drivers/clk/tegra/clk-tegra-super-gen4.c
> +++ b/drivers/clk/tegra/clk-tegra-super-gen4.c
> @@ -180,7 +180,7 @@ static void __init tegra_super_clk_init(void __iomem *clk_base,
> gen_info->num_cclk_g_parents,
> CLK_SET_RATE_PARENT,
> clk_base + CCLKG_BURST_POLICY,
> - 0, 4, 8, 0, NULL);
> + TEGRA210_CPU_CLK, 4, 8, 0, NULL);
> } else {
> clk = tegra_clk_register_super_mux("cclk_g",
> gen_info->cclk_g_parents,
> @@ -196,6 +196,11 @@ static void __init tegra_super_clk_init(void __iomem *clk_base,
> dt_clk = tegra_lookup_dt_id(tegra_clk_cclk_lp, tegra_clks);
> if (dt_clk) {
> if (gen_info->gen == gen5) {
> + /*
> + * TEGRA210_CPU_CLK flag is not needed for cclk_lp as cluster
> + * switching is not currently supported on Tegra210 and also
> + * cpu_lp is not used.
> + */
> clk = tegra_clk_register_super_mux("cclk_lp",
> gen_info->cclk_lp_parents,
> gen_info->num_cclk_lp_parents,
> diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c
> index 573e3c967ae1..eb08047fd02f 100644
> --- a/drivers/clk/tegra/clk.c
> +++ b/drivers/clk/tegra/clk.c
> @@ -23,6 +23,7 @@
> #define CLK_OUT_ENB_W 0x364
> #define CLK_OUT_ENB_X 0x280
> #define CLK_OUT_ENB_Y 0x298
> +#define CLK_ENB_PLLP_OUT_CPU BIT(31)
> #define CLK_OUT_ENB_SET_L 0x320
> #define CLK_OUT_ENB_CLR_L 0x324
> #define CLK_OUT_ENB_SET_H 0x328
> @@ -199,6 +200,19 @@ const struct tegra_clk_periph_regs *get_reg_bank(int clkid)
> }
> }
>
> +void tegra_clk_set_pllp_out_cpu(bool enable)
> +{
> + u32 val;
> +
> + val = readl_relaxed(clk_base + CLK_OUT_ENB_Y);
> + if (enable)
> + val |= CLK_ENB_PLLP_OUT_CPU;
> + else
> + val &= ~CLK_ENB_PLLP_OUT_CPU;
> +
> + writel_relaxed(val, clk_base + CLK_OUT_ENB_Y);
> +}
> +
> struct clk ** __init tegra_clk_init(void __iomem *regs, int num, int banks)
> {
> clk_base = regs;
> diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
> index 8a9af45b6084..560e2bcb3d7d 100644
> --- a/drivers/clk/tegra/clk.h
> +++ b/drivers/clk/tegra/clk.h
> @@ -677,6 +677,9 @@ struct clk *tegra_clk_register_periph_data(void __iomem *clk_base,
> * Flags:
> * TEGRA_DIVIDER_2 - LP cluster has additional divider. This flag indicates
> * that this is LP cluster clock.
> + * TEGRA210_CPU_CLK - This flag is used to identify CPU cluster for gen5
> + * super mux parent using PLLP branches. To use PLLP branches to CPU, need
> + * to configure additional bit PLLP_OUT_CPU in the clock registers.
> */
> struct tegra_clk_super_mux {
> struct clk_hw hw;
> @@ -693,6 +696,7 @@ struct tegra_clk_super_mux {
> #define to_clk_super_mux(_hw) container_of(_hw, struct tegra_clk_super_mux, hw)
>
> #define TEGRA_DIVIDER_2 BIT(0)
> +#define TEGRA210_CPU_CLK BIT(1)
>
> extern const struct clk_ops tegra_clk_super_ops;
> struct clk *tegra_clk_register_super_mux(const char *name,
> @@ -838,6 +842,7 @@ int tegra_pll_p_div_to_hw(struct tegra_clk_pll *pll, u8 p_div);
> int div_frac_get(unsigned long rate, unsigned parent_rate, u8 width,
> u8 frac_width, u8 flags);
> void tegra_clk_osc_resume(void __iomem *clk_base);
> +void tegra_clk_set_pllp_out_cpu(bool enable);
>
>
> /* Combined read fence with delay */
>
Looks good to me.
Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
^ permalink raw reply
* Re: [PATCH v2 15/15] dt-bindings: net: add bindings for ADIN PHY driver
From: Ardelean, Alexandru @ 2019-08-09 12:06 UTC (permalink / raw)
To: robh+dt@kernel.org
Cc: andrew@lunn.ch, davem@davemloft.net, hkallweit1@gmail.com,
devicetree@vger.kernel.org, mark.rutland@arm.com,
linux-kernel@vger.kernel.org, f.fainelli@gmail.com,
netdev@vger.kernel.org
In-Reply-To: <CAL_Jsq+zH9cL5-8aDARzPar+xoD71WbESTckGjzaUTodu-+Trg@mail.gmail.com>
On Thu, 2019-08-08 at 17:03 -0600, Rob Herring wrote:
> [External]
>
> On Thu, Aug 8, 2019 at 6:31 AM Alexandru Ardelean
> <alexandru.ardelean@analog.com> wrote:
> > This change adds bindings for the Analog Devices ADIN PHY driver, detailing
> > all the properties implemented by the driver.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> > .../devicetree/bindings/net/adi,adin.yaml | 76 +++++++++++++++++++
> > MAINTAINERS | 1 +
> > 2 files changed, 77 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/net/adi,adin.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml
> > b/Documentation/devicetree/bindings/net/adi,adin.yaml
> > new file mode 100644
> > index 000000000000..86177c8fe23a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
> > @@ -0,0 +1,76 @@
> > +# SPDX-License-Identifier: GPL-2.0+
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/adi,adin.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices ADIN1200/ADIN1300 PHY
> > +
> > +maintainers:
> > + - Alexandru Ardelean <alexandru.ardelean@analog.com>
> > +
> > +description: |
> > + Bindings for Analog Devices Industrial Ethernet PHYs
> > +
> > +allOf:
> > + - $ref: ethernet-phy.yaml#
> > +
> > +properties:
> > + adi,rx-internal-delay-ps:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: |
> > + RGMII RX Clock Delay used only when PHY operates in RGMII mode with
> > + internal delay (phy-mode is 'rgmii-id' or 'rgmii-rxid') in pico-seconds.
> > + enum: [ 1600, 1800, 2000, 2200, 2400 ]
> > + default: 2000
>
> This doesn't actually do what you think. The '$ref' has to be under an
> 'allOf' to work. It's an oddity of json-schema. However, anything with
> a standard unit suffix already has a schema to define the type, so you
> don't need to here and can just drop $ref.
ack
will drop
>
> > +
> > + adi,tx-internal-delay-ps:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: |
> > + RGMII TX Clock Delay used only when PHY operates in RGMII mode with
> > + internal delay (phy-mode is 'rgmii-id' or 'rgmii-txid') in pico-seconds.
> > + enum: [ 1600, 1800, 2000, 2200, 2400 ]
> > + default: 2000
> > +
> > + adi,fifo-depth-bits:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: |
> > + When operating in RMII mode, this option configures the FIFO depth.
> > + enum: [ 4, 8, 12, 16, 20, 24 ]
> > + default: 8
> > +
> > + adi,disable-energy-detect:
> > + description: |
> > + Disables Energy Detect Powerdown Mode (default disabled, i.e energy detect
> > + is enabled if this property is unspecified)
> > + type: boolean
> > +
> > +examples:
> > + - |
> > + ethernet {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + phy-mode = "rgmii-id";
> > +
> > + ethernet-phy@0 {
> > + reg = <0>;
> > +
> > + adi,rx-internal-delay-ps = <1800>;
> > + adi,tx-internal-delay-ps = <2200>;
> > + };
> > + };
> > + - |
> > + ethernet {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + phy-mode = "rmii";
> > +
> > + ethernet-phy@1 {
> > + reg = <1>;
> > +
> > + adi,fifo-depth-bits = <16>;
> > + adi,disable-energy-detect;
> > + };
> > + };
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index e8aa8a667864..fd9ab61c2670 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -944,6 +944,7 @@ L: netdev@vger.kernel.org
> > W: http://ez.analog.com/community/linux-device-drivers
> > S: Supported
> > F: drivers/net/phy/adin.c
> > +F: Documentation/devicetree/bindings/net/adi,adin.yaml
> >
> > ANALOG DEVICES INC ADIS DRIVER LIBRARY
> > M: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > --
> > 2.20.1
> >
^ permalink raw reply
* Re: [PATCH v2 13/15] net: phy: adin: configure downshift on config_init
From: Ardelean, Alexandru @ 2019-08-09 12:06 UTC (permalink / raw)
To: hkallweit1@gmail.com, andrew@lunn.ch
Cc: f.fainelli@gmail.com, mark.rutland@arm.com,
devicetree@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, robh+dt@kernel.org,
davem@davemloft.net
In-Reply-To: <20190808203932.GP27917@lunn.ch>
On Thu, 2019-08-08 at 22:39 +0200, Andrew Lunn wrote:
> [External]
>
> On Thu, Aug 08, 2019 at 09:38:40PM +0200, Heiner Kallweit wrote:
> > On 08.08.2019 14:30, Alexandru Ardelean wrote:
> > > Down-speed auto-negotiation may not always be enabled, in which case the
> > > PHY won't down-shift to 100 or 10 during auto-negotiation.
> > >
> > > This change enables downshift and configures the number of retries to
> > > default 8 (maximum supported value).
> > >
> > > The change has been adapted from the Marvell PHY driver.
> > >
> > Instead of a fixed downshift setting (like in the Marvell driver) you
> > may consider to implement the ethtool phy-tunable ETHTOOL_PHY_DOWNSHIFT.
>
> Hi Alexandru
>
> Upps, sorry, my bad.
>
> I looked at marvell_set_downshift(), and assumed it was connected to
> the phy-tunable. I have patches somewhere which does that. But they
> have not made it into mainline yet.
>
> > See the Aquantia PHY driver for an example.
>
> Yes, that does have all the tunable stuff.
Ack.
Will use that
>
> Andrew
^ permalink raw reply
* Re: [PATCH v8 09/14] media: rkisp1: add rockchip isp1 core driver
From: Sakari Ailus @ 2019-08-09 12:05 UTC (permalink / raw)
To: Helen Koike
Cc: linux-rockchip, devicetree, eddie.cai.linux, mchehab, heiko,
jacob2.chen, jeffy.chen, zyc, linux-kernel, tfiga, hans.verkuil,
laurent.pinchart, kernel, ezequiel, linux-media, linux-arm-kernel,
zhengsq, Jacob Chen, Allon Huang
In-Reply-To: <bca5e3fe-8a37-2b91-ae96-30378fd98012@collabora.com>
Hi Helen,
On Thu, Aug 08, 2019 at 06:59:47PM -0300, Helen Koike wrote:
> Hi Sakari,
>
> Thanks for your review, just some comments/questions below:
>
> On 8/7/19 12:27 PM, Sakari Ailus wrote:
> > Hi Helen,
> >
> > On Tue, Jul 30, 2019 at 03:42:51PM -0300, Helen Koike wrote:
> >> From: Jacob Chen <jacob2.chen@rock-chips.com>
> >>
> >> Add the core driver for rockchip isp1.
> >>
> >> Signed-off-by: Jacob Chen <jacob2.chen@rock-chips.com>
> >> Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
> >> Signed-off-by: Yichong Zhong <zyc@rock-chips.com>
> >> Signed-off-by: Jacob Chen <cc@rock-chips.com>
> >> Signed-off-by: Eddie Cai <eddie.cai.linux@gmail.com>
> >> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> >> Signed-off-by: Allon Huang <allon.huang@rock-chips.com>
> >> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
> >> [fixed compilation and run time errors regarding new v4l2 async API]
> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> [Add missing module device table]
> >> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> >> [update for upstream]
> >> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> >>
> >> ---
> >>
> >> Changes in v8: None
> >> Changes in v7:
> >> - VIDEO_ROCKCHIP_ISP1 selects VIDEOBUF2_VMALLOC
> >> - add PHY_ROCKCHIP_DPHY as a dependency for VIDEO_ROCKCHIP_ISP1
> >> - Fix compilation and runtime errors due to bitrotting
> >> The code has bit-rotten since March 2018, fix compilation errors.
> >> The new V4L2 async notifier API requires notifiers to be initialized by
> >> a call to v4l2_async_notifier_init() before being used, do so.
> >> - Add missing module device table
> >> - use clk_bulk framework
> >> - add missing notifiers cleanups
> >> - s/strlcpy/strscpy
> >> - normalize bus_info name
> >> - fix s_stream error path, stream_cnt wans't being decremented properly
> >> - use devm_platform_ioremap_resource() helper
> >> - s/deice/device
> >> - redesign: remove mipi/csi subdevice, sensors connect directly to the
> >> isp subdevice in the media topology now.
> >> - remove "saved_state" member from rkisp1_stream struct
> >> - Reverse the order of MIs
> >> - Simplify MI interrupt handling
> >> Rather than adding unnecessary indirection, just use stream index to
> >> handle MI interrupt enable/disable/clear, since the stream index matches
> >> the order of bits now, thanks to previous patch. While at it, remove
> >> some dead code.
> >> - code styling and checkpatch fixes
> >>
> >> drivers/media/platform/Kconfig | 12 +
> >> drivers/media/platform/Makefile | 1 +
> >> drivers/media/platform/rockchip/isp1/Makefile | 7 +
> >> drivers/media/platform/rockchip/isp1/common.h | 101 +++
> >> drivers/media/platform/rockchip/isp1/dev.c | 675 ++++++++++++++++++
> >> drivers/media/platform/rockchip/isp1/dev.h | 97 +++
> >> 6 files changed, 893 insertions(+)
> >> create mode 100644 drivers/media/platform/rockchip/isp1/Makefile
> >> create mode 100644 drivers/media/platform/rockchip/isp1/common.h
> >> create mode 100644 drivers/media/platform/rockchip/isp1/dev.c
> >> create mode 100644 drivers/media/platform/rockchip/isp1/dev.h
> >>
> >> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> >> index 89555f9a813f..e0e98937c565 100644
> >> --- a/drivers/media/platform/Kconfig
> >> +++ b/drivers/media/platform/Kconfig
> >> @@ -106,6 +106,18 @@ config VIDEO_QCOM_CAMSS
> >> select VIDEOBUF2_DMA_SG
> >> select V4L2_FWNODE
> >>
> >> +config VIDEO_ROCKCHIP_ISP1
> >> + tristate "Rockchip Image Signal Processing v1 Unit driver"
> >> + depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> >> + depends on ARCH_ROCKCHIP || COMPILE_TEST
> >> + select VIDEOBUF2_DMA_CONTIG
> >> + select VIDEOBUF2_VMALLOC
> >> + select V4L2_FWNODE
> >> + select PHY_ROCKCHIP_DPHY
> >> + default n
> >> + ---help---
> >> + Support for ISP1 on the rockchip SoC.
> >> +
> >> config VIDEO_S3C_CAMIF
> >> tristate "Samsung S3C24XX/S3C64XX SoC Camera Interface driver"
> >> depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
> >> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> >> index 7cbbd925124c..f9fcf8e7c513 100644
> >> --- a/drivers/media/platform/Makefile
> >> +++ b/drivers/media/platform/Makefile
> >> @@ -69,6 +69,7 @@ obj-$(CONFIG_VIDEO_RENESAS_FDP1) += rcar_fdp1.o
> >> obj-$(CONFIG_VIDEO_RENESAS_JPU) += rcar_jpu.o
> >> obj-$(CONFIG_VIDEO_RENESAS_VSP1) += vsp1/
> >>
> >> +obj-$(CONFIG_VIDEO_ROCKCHIP_ISP1) += rockchip/isp1/
> >> obj-$(CONFIG_VIDEO_ROCKCHIP_RGA) += rockchip/rga/
> >>
> >> obj-y += omap/
> >> diff --git a/drivers/media/platform/rockchip/isp1/Makefile b/drivers/media/platform/rockchip/isp1/Makefile
> >> new file mode 100644
> >> index 000000000000..72706e80fc8b
> >> --- /dev/null
> >> +++ b/drivers/media/platform/rockchip/isp1/Makefile
> >> @@ -0,0 +1,7 @@
> >> +obj-$(CONFIG_VIDEO_ROCKCHIP_ISP1) += rockchip-isp1.o
> >> +rockchip-isp1-objs += rkisp1.o \
> >> + dev.o \
> >> + regs.o \
> >> + isp_stats.o \
> >> + isp_params.o \
> >> + capture.o
> >> diff --git a/drivers/media/platform/rockchip/isp1/common.h b/drivers/media/platform/rockchip/isp1/common.h
> >> new file mode 100644
> >> index 000000000000..606ce2793546
> >> --- /dev/null
> >> +++ b/drivers/media/platform/rockchip/isp1/common.h
> >> @@ -0,0 +1,101 @@
> >> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> >> +/*
> >> + * Rockchip isp1 driver
> >> + *
> >> + * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
> >> + */
> >> +
> >> +#ifndef _RKISP1_COMMON_H
> >> +#define _RKISP1_COMMON_H
> >> +
> >> +#include <linux/mutex.h>
> >> +#include <media/media-device.h>
> >> +#include <media/media-entity.h>
> >> +#include <media/v4l2-ctrls.h>
> >> +#include <media/v4l2-device.h>
> >> +#include <media/videobuf2-v4l2.h>
> >> +
> >> +#define RKISP1_DEFAULT_WIDTH 800
> >> +#define RKISP1_DEFAULT_HEIGHT 600
> >> +
> >> +#define RKISP1_MAX_STREAM 2
> >> +#define RKISP1_STREAM_MP 0
> >> +#define RKISP1_STREAM_SP 1
> >> +
> >> +#define RKISP1_PLANE_Y 0
> >> +#define RKISP1_PLANE_CB 1
> >> +#define RKISP1_PLANE_CR 2
> >> +
> >> +enum rkisp1_sd_type {
> >> + RKISP1_SD_SENSOR,
> >> + RKISP1_SD_PHY_CSI,
> >> + RKISP1_SD_VCM,
> >> + RKISP1_SD_FLASH,
> >> + RKISP1_SD_MAX,
> >> +};
> >
> > I wonder if this is a leftover from the driver development time. Same goes
> > for the subdevs field in struct rkisp1_device.
>
> It is a left over, I'm removing them for the next version, thanks.
>
> >
> >> +
> >> +/* One structure per video node */
> >> +struct rkisp1_vdev_node {
> >> + struct vb2_queue buf_queue;
> >> + /* vfd lock */
> >> + struct mutex vlock;
> >> + struct video_device vdev;
> >> + struct media_pad pad;
> >> +};
> >> +
> >> +enum rkisp1_fmt_pix_type {
> >> + FMT_YUV,
> >> + FMT_RGB,
> >> + FMT_BAYER,
> >> + FMT_JPEG,
> >> + FMT_MAX
> >> +};
> >> +
> >> +enum rkisp1_fmt_raw_pat_type {
> >> + RAW_RGGB = 0,
> >> + RAW_GRBG,
> >> + RAW_GBRG,
> >> + RAW_BGGR,
> >> +};
> >> +
> >> +struct rkisp1_buffer {
> >> + struct vb2_v4l2_buffer vb;
> >> + struct list_head queue;
> >> + union {
> >> + u32 buff_addr[VIDEO_MAX_PLANES];
> >> + void *vaddr[VIDEO_MAX_PLANES];
> >> + };
> >> +};
> >> +
> >> +struct rkisp1_dummy_buffer {
> >> + void *vaddr;
> >> + dma_addr_t dma_addr;
> >> + u32 size;
> >> +};
> >> +
> >> +extern int rkisp1_debug;
> >> +
> >> +static inline
> >> +struct rkisp1_vdev_node *vdev_to_node(struct video_device *vdev)
> >> +{
> >> + return container_of(vdev, struct rkisp1_vdev_node, vdev);
> >> +}
> >> +
> >> +static inline struct rkisp1_vdev_node *queue_to_node(struct vb2_queue *q)
> >> +{
> >> + return container_of(q, struct rkisp1_vdev_node, buf_queue);
> >> +}
> >> +
> >> +static inline struct rkisp1_buffer *to_rkisp1_buffer(struct vb2_v4l2_buffer *vb)
> >> +{
> >> + return container_of(vb, struct rkisp1_buffer, vb);
> >> +}
> >> +
> >> +static inline struct vb2_queue *to_vb2_queue(struct file *file)
> >> +{
> >> + struct rkisp1_vdev_node *vnode = video_drvdata(file);
> >> +
> >> + return &vnode->buf_queue;
> >> +}
> >> +
> >> +#endif /* _RKISP1_COMMON_H */
> >> diff --git a/drivers/media/platform/rockchip/isp1/dev.c b/drivers/media/platform/rockchip/isp1/dev.c
> >> new file mode 100644
> >> index 000000000000..2b4a67e1a3b5
> >> --- /dev/null
> >> +++ b/drivers/media/platform/rockchip/isp1/dev.c
> >> @@ -0,0 +1,675 @@
> >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> >> +/*
> >> + * Rockchip isp1 driver
> >> + *
> >> + * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
> >> + */
> >> +
> >> +#include <linux/clk.h>
> >> +#include <linux/interrupt.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_graph.h>
> >> +#include <linux/of_platform.h>
> >> +#include <linux/pm_runtime.h>
> >> +#include <linux/pinctrl/consumer.h>
> >> +#include <linux/phy/phy.h>
> >> +#include <linux/phy/phy-mipi-dphy.h>
> >> +
> >> +#include "common.h"
> >> +#include "regs.h"
> >> +
> >> +struct isp_match_data {
> >> + const char * const *clks;
> >> + int size;
> >
> > unsigned int
>
> ack
>
> >
> >> +};
> >> +
> >> +struct sensor_async_subdev {
> >> + struct v4l2_async_subdev asd;
> >> + struct v4l2_mbus_config mbus;
> >> + unsigned int lanes;
> >> +};
> >> +
> >> +int rkisp1_debug;
> >> +module_param_named(debug, rkisp1_debug, int, 0644);
> >> +MODULE_PARM_DESC(debug, "Debug level (0-1)");
> >
> > Have you thought of using dynamic debug instead?
>
> right, this is being used in v4l2_dbg(), which I can replace by dev_dbg()
> that is already covered by dynamic debug iirc.
> Should I also replace v4l2_err() by dev_err() (I always get confused by
> which log function I should use).
In case you switch to the dev_*() macros, then yes. The corresponding
v4l2_*() macros are still an entirely valid interface to use. But today we
likely wouldn't add such macros; I usually ask driver developers using both
to switch to the dev_*() ones.
>
> >
> >> +
> >> +/**************************** pipeline operations******************************/
> >> +
> >> +static int __isp_pipeline_prepare(struct rkisp1_pipeline *p,
> >> + struct media_entity *me)
> >> +{
> >> + struct rkisp1_device *dev = container_of(p, struct rkisp1_device, pipe);
> >> + struct v4l2_subdev *sd;
> >> + unsigned int i;
> >> +
> >> + p->num_subdevs = 0;
> >> + memset(p->subdevs, 0, sizeof(p->subdevs));
> >> +
> >> + while (1) {
> >> + struct media_pad *pad = NULL;
> >> +
> >> + /* Find remote source pad */
> >> + for (i = 0; i < me->num_pads; i++) {
> >> + struct media_pad *spad = &me->pads[i];
> >> +
> >> + if (!(spad->flags & MEDIA_PAD_FL_SINK))
> >> + continue;
> >> + pad = media_entity_remote_pad(spad);
> >> + if (pad)
> >> + break;
> >> + }
> >> +
> >> + if (!pad)
> >> + break;
> >> +
> >> + sd = media_entity_to_v4l2_subdev(pad->entity);
> >> + if (sd != &dev->isp_sdev.sd)
> >> + p->subdevs[p->num_subdevs++] = sd;
> >
> > How do you make sure you don't overrun the array?
>
> Because the maximum path the topology can have is:
> sensor->rkisp->capture
The sensor may consist of more than one sub-device, and there may be
bridges such as parallel/CSI-2 ones in between. The latter would be an
unlikely hardware configuration for this ISP though as it supports both.
External ISPs in front of the camera sensor are another possibility.
Basically you can't make expectations of the topology outside the scope of
your own device --- the topology of which is known.
>
> >
> > Instead, I'd avoid maintaining the array in the first place --- the same
> > information is available from the MC framework data structures --- see e.g.
> > the omap3isp driver.
>
> If I understand correctly, omap3isp navigates through the topology in the same way,
> but it just don't save in an array, but I reuse this information in other places,
> mostly for power up/down (see below why I don't use v4l2_pipeline_pm_use())
Correct.
>
> >
> >> +
> >> + me = &sd->entity;
> >> + if (me->num_pads == 1)
> >> + break;
> >> + }
> >> + return 0;
> >> +}
> >> +
> >> +static int __subdev_set_power(struct v4l2_subdev *sd, int on)
> >> +{
> >> + int ret;
> >> +
> >> + if (!sd)
> >> + return -ENXIO;
> >> +
> >> + ret = v4l2_subdev_call(sd, core, s_power, on);
> >> +
> >> + return ret != -ENOIOCTLCMD ? ret : 0;
> >> +}
> >> +
> >> +static int __isp_pipeline_s_power(struct rkisp1_pipeline *p, bool on)
> >
> > Could you instead use v4l2_pipeline_pm_use()?
>
> Unless I misunderstood this (which is very likely), v4l2_pipeline_pm_use() calls pipeline_pm_power(),
> that applies power change to all entities in a pipeline, but if I have two sensors
> connected to the ISP, one with link enabled and the other with a disabled link,
> I don't want to power both sensors on, just the one participating in that stream. And
> if I call v4l2_pipeline_pm_use() it will power on both, which is not what I want.
>
> Does it make sense?
The v4l2_pipeline_pm_use() only follows the active links. A simplistic
implementation could power on all the subdevs in the graph but no; the
v4l2_pipeline_pm_use() exists for this very purpose.
>
> >
> >> +{
> >> + struct rkisp1_device *dev = container_of(p, struct rkisp1_device, pipe);
> >> + int i, ret;
> >> +
> >> + if (on) {
> >> + __subdev_set_power(&dev->isp_sdev.sd, true);
> >> +
> >> + for (i = p->num_subdevs - 1; i >= 0; --i) {
> >> + ret = __subdev_set_power(p->subdevs[i], true);
> >> + if (ret < 0 && ret != -ENXIO)
> >> + goto err_power_off;
> >> + }
> >> + } else {
> >> + for (i = 0; i < p->num_subdevs; ++i)
> >> + __subdev_set_power(p->subdevs[i], false);
> >> +
> >> + __subdev_set_power(&dev->isp_sdev.sd, false);
> >> + }
> >> +
> >> + return 0;
> >> +
> >> +err_power_off:
> >> + for (++i; i < p->num_subdevs; ++i)
> >> + __subdev_set_power(p->subdevs[i], false);
> >> + __subdev_set_power(&dev->isp_sdev.sd, true);
> >> + return ret;
> >> +}
> >> +
> >> +static int rkisp1_pipeline_open(struct rkisp1_pipeline *p,
> >> + struct media_entity *me,
> >> + bool prepare)
> >> +{
> >> + int ret;
> >> +
> >> + if (WARN_ON(!p || !me))
> >> + return -EINVAL;
> >> + if (atomic_inc_return(&p->power_cnt) > 1)
> >> + return 0;
> >> +
> >> + /* go through media graphic and get subdevs */
> >> + if (prepare)
> >> + __isp_pipeline_prepare(p, me);
> >> +
> >> + if (!p->num_subdevs)
> >> + return -EINVAL;
> >> +
> >> + ret = __isp_pipeline_s_power(p, 1);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int rkisp1_pipeline_close(struct rkisp1_pipeline *p)
> >> +{
> >> + int ret;
> >> +
> >> + if (atomic_dec_return(&p->power_cnt) > 0)
> >> + return 0;
> >> + ret = __isp_pipeline_s_power(p, 0);
> >> +
> >> + return ret == -ENXIO ? 0 : ret;
> >> +}
> >> +
> >> +/*
> >> + * stream-on order: isp_subdev, mipi dphy, sensor
> >> + * stream-off order: mipi dphy, sensor, isp_subdev
> >> + */
> >> +static int rkisp1_pipeline_set_stream(struct rkisp1_pipeline *p, bool on)
> >> +{
> >> + struct rkisp1_device *dev = container_of(p, struct rkisp1_device, pipe);
> >> + int i, ret;
> >> +
> >> + if ((on && atomic_inc_return(&p->stream_cnt) > 1) ||
> >> + (!on && atomic_dec_return(&p->stream_cnt) > 0))
> >> + return 0;
> >> +
> >> + if (on) {
> >> + ret = v4l2_subdev_call(&dev->isp_sdev.sd, video, s_stream,
> >> + true);
> >> + if (ret && ret != -ENOIOCTLCMD && ret != -ENODEV) {
> >> + v4l2_err(&dev->v4l2_dev,
> >> + "s_stream failed on subdevice %s (%d)\n",
> >> + dev->isp_sdev.sd.name,
> >> + ret);
> >> + atomic_dec(&p->stream_cnt);
> >> + return ret;
> >> + }
> >> + }
> >> +
> >> + /* phy -> sensor */
> >> + for (i = 0; i < p->num_subdevs; ++i) {
> >> + ret = v4l2_subdev_call(p->subdevs[i], video, s_stream, on);
> >> + if (on && ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> >> + goto err_stream_off;
> >
> > You should stop after the first external sub-device.
> >
> > It seems even the omap3isp driver doesn't do that. It's not easy to spot
> > such issues indeed.
>
> I'm not sure I understand, what do you call an external sub-device? Is the sensor
> an external subdevice?
One that is not controlled by this driver.
>
> >
> >> + }
> >> +
> >> + if (!on)
> >> + v4l2_subdev_call(&dev->isp_sdev.sd, video, s_stream, false);
> >> +
> >> + return 0;
> >> +
> >> +err_stream_off:
> >> + for (--i; i >= 0; --i)
> >> + v4l2_subdev_call(p->subdevs[i], video, s_stream, false);
> >> + v4l2_subdev_call(&dev->isp_sdev.sd, video, s_stream, false);
> >> + atomic_dec(&p->stream_cnt);
> >> + return ret;
> >> +}
> >> +
> >> +/***************************** media controller *******************************/
> >> +/* See http://opensource.rock-chips.com/wiki_Rockchip-isp1 for Topology */
> >
> > The host appears to be down, or there's a routing problem. Unless this is
> > fixed, having such a URL here doesn't do much good. :-I
>
> This is a left over, sorry about that.
> I can access the URL now. I'll try to get some of the docs and move to the kernel docs.
I wonder if the server is only powered on during the local office hours.
;-)
More seriously, I looked into this and it seems there's a routing problem
somewhere between my ISP and that server. From a few other places the
server is reachable.
>
> >
> >> +
> >> +static int rkisp1_create_links(struct rkisp1_device *dev)
> >> +{
> >> + struct media_entity *source, *sink;
> >> + struct rkisp1_sensor *sensor;
> >> + unsigned int flags, pad;
> >> + int ret;
> >> +
> >> + /* sensor links(or mipi-phy) */
> >> + list_for_each_entry(sensor, &dev->sensors, list) {
> >> + for (pad = 0; pad < sensor->sd->entity.num_pads; pad++)
> >> + if (sensor->sd->entity.pads[pad].flags &
> >> + MEDIA_PAD_FL_SOURCE)
> >> + break;
> >
> > Could you use media_entity_get_fwnode_pad() instead?
>
> Yes, I didn't know about it actually, thanks for that, looks cleaner (I'll send in
> the next version).
>
> >
> >> +
> >> + if (pad == sensor->sd->entity.num_pads) {
> >> + dev_err(dev->dev,
> >> + "failed to find src pad for %s\n",
> >> + sensor->sd->name);
> >> +
> >> + return -ENXIO;
> >> + }
> >> +
> >> + ret = media_create_pad_link(
> >> + &sensor->sd->entity, pad,
> >> + &dev->isp_sdev.sd.entity,
> >> + RKISP1_ISP_PAD_SINK,
> >> + list_is_first(&sensor->list, &dev->sensors) ?
> >> + MEDIA_LNK_FL_ENABLED : 0);
> >> + if (ret) {
> >> + dev_err(dev->dev,
> >> + "failed to create link for %s\n",
> >> + sensor->sd->name);
> >> + return ret;
> >> + }
> >> + }
> >> +
> >> + /* params links */
> >> + source = &dev->params_vdev.vnode.vdev.entity;
> >> + sink = &dev->isp_sdev.sd.entity;
> >> + flags = MEDIA_LNK_FL_ENABLED;
> >> + ret = media_create_pad_link(source, 0, sink,
> >> + RKISP1_ISP_PAD_SINK_PARAMS, flags);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + /* create isp internal links */
> >> + /* SP links */
> >> + source = &dev->isp_sdev.sd.entity;
> >> + sink = &dev->stream[RKISP1_STREAM_SP].vnode.vdev.entity;
> >> + ret = media_create_pad_link(source, RKISP1_ISP_PAD_SOURCE_PATH,
> >> + sink, 0, flags);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + /* MP links */
> >> + source = &dev->isp_sdev.sd.entity;
> >> + sink = &dev->stream[RKISP1_STREAM_MP].vnode.vdev.entity;
> >> + ret = media_create_pad_link(source, RKISP1_ISP_PAD_SOURCE_PATH,
> >> + sink, 0, flags);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + /* 3A stats links */
> >> + source = &dev->isp_sdev.sd.entity;
> >> + sink = &dev->stats_vdev.vnode.vdev.entity;
> >> + return media_create_pad_link(source, RKISP1_ISP_PAD_SOURCE_STATS,
> >> + sink, 0, flags);
> >
> > Indentation. Same for the calls to the same function above.
>
> ack
>
> >
> >> +}
> >> +
> >> +static int subdev_notifier_bound(struct v4l2_async_notifier *notifier,
> >> + struct v4l2_subdev *sd,
> >> + struct v4l2_async_subdev *asd)
> >> +{
> >> + struct rkisp1_device *isp_dev = container_of(notifier,
> >> + struct rkisp1_device,
> >> + notifier);
> >> + struct sensor_async_subdev *s_asd = container_of(asd,
> >> + struct sensor_async_subdev, asd);
> >> + struct rkisp1_sensor *sensor;
> >> +
> >> + sensor = devm_kzalloc(isp_dev->dev, sizeof(*sensor), GFP_KERNEL);
> >> + if (!sensor)
> >> + return -ENOMEM;
> >> +
> >> + sensor->lanes = s_asd->lanes;
> >> + sensor->mbus = s_asd->mbus;
> >> + sensor->sd = sd;
> >> + sensor->dphy = devm_phy_get(isp_dev->dev, "dphy");
> >> + if (IS_ERR(sensor->dphy)) {
> >> + if (PTR_ERR(sensor->dphy) != -EPROBE_DEFER)
> >> + dev_err(isp_dev->dev, "Couldn't get the MIPI D-PHY\n");
> >> + return PTR_ERR(sensor->dphy);
> >> + }
> >> + phy_init(sensor->dphy);
> >> +
> >> + list_add(&sensor->list, &isp_dev->sensors);
> >
> > In general, maintaining the information on the external subdevs on your own
> > adds complexity to the driver. You can get the information when you need it
> > from the data structures maintained by MC (see e.g. the omap3isp driver for
> > examples).
> >
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static struct rkisp1_sensor *sd_to_sensor(struct rkisp1_device *dev,
> >> + struct v4l2_subdev *sd)
> >> +{
> >> + struct rkisp1_sensor *sensor;
> >> +
> >> + list_for_each_entry(sensor, &dev->sensors, list)
> >> + if (sensor->sd == sd)
> >> + return sensor;
> >> +
> >> + return NULL;
> >> +}
> >> +
> >> +static void subdev_notifier_unbind(struct v4l2_async_notifier *notifier,
> >> + struct v4l2_subdev *sd,
> >> + struct v4l2_async_subdev *asd)
> >> +{
> >> + struct rkisp1_device *isp_dev = container_of(notifier,
> >> + struct rkisp1_device,
> >> + notifier);
> >> + struct rkisp1_sensor *sensor = sd_to_sensor(isp_dev, sd);
> >> +
> >> + /* TODO: check if a lock is required here */
> >> + list_del(&sensor->list);
> >> +
> >> + phy_exit(sensor->dphy);
> >> +}
> >> +
> >> +static int subdev_notifier_complete(struct v4l2_async_notifier *notifier)
> >> +{
> >> + struct rkisp1_device *dev = container_of(notifier, struct rkisp1_device,
> >> + notifier);
> >> + int ret;
> >> +
> >> + mutex_lock(&dev->media_dev.graph_mutex);
> >> + ret = rkisp1_create_links(dev);
> >> + if (ret < 0)
> >> + goto unlock;
> >> + ret = v4l2_device_register_subdev_nodes(&dev->v4l2_dev);
> >> + if (ret < 0)
> >> + goto unlock;
> >> +
> >> + v4l2_info(&dev->v4l2_dev, "Async subdev notifier completed\n");
> >> +
> >> +unlock:
> >> + mutex_unlock(&dev->media_dev.graph_mutex);
> >> + return ret;
> >> +}
> >> +
> >> +static int rkisp1_fwnode_parse(struct device *dev,
> >> + struct v4l2_fwnode_endpoint *vep,
> >> + struct v4l2_async_subdev *asd)
> >> +{
> >> + struct sensor_async_subdev *s_asd =
> >> + container_of(asd, struct sensor_async_subdev, asd);
> >> +
> >> + if (vep->bus_type != V4L2_MBUS_CSI2_DPHY) {
> >> + dev_err(dev, "Only CSI2 bus type is currently supported\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + if (vep->base.port != 0) {
> >> + dev_err(dev, "The ISP has only port 0\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + s_asd->mbus.type = vep->bus_type;
> >> + s_asd->mbus.flags = vep->bus.mipi_csi2.flags;
> >> + s_asd->lanes = vep->bus.mipi_csi2.num_data_lanes;
> >> +
> >> + switch (vep->bus.mipi_csi2.num_data_lanes) {
> >> + case 1:
> >> + s_asd->mbus.flags |= V4L2_MBUS_CSI2_1_LANE;
> >> + break;
> >> + case 2:
> >> + s_asd->mbus.flags |= V4L2_MBUS_CSI2_2_LANE;
> >> + break;
> >> + case 3:
> >> + s_asd->mbus.flags |= V4L2_MBUS_CSI2_3_LANE;
> >> + break;
> >> + case 4:
> >> + s_asd->mbus.flags |= V4L2_MBUS_CSI2_4_LANE;
> >> + break;
> >> + default:
> >> + return -EINVAL;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static const struct v4l2_async_notifier_operations subdev_notifier_ops = {
> >> + .bound = subdev_notifier_bound,
> >> + .unbind = subdev_notifier_unbind,
> >> + .complete = subdev_notifier_complete,
> >> +};
> >> +
> >> +static int isp_subdev_notifier(struct rkisp1_device *isp_dev)
> >> +{
> >> + struct v4l2_async_notifier *ntf = &isp_dev->notifier;
> >> + struct device *dev = isp_dev->dev;
> >> + int ret;
> >> +
> >> + v4l2_async_notifier_init(ntf);
> >> +
> >> + ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> >> + dev, ntf, sizeof(struct sensor_async_subdev), 0,
> >> + rkisp1_fwnode_parse);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + if (list_empty(&ntf->asd_list))
> >> + return -ENODEV; /* no endpoint */
> >> +
> >> + ntf->ops = &subdev_notifier_ops;
> >> +
> >> + return v4l2_async_notifier_register(&isp_dev->v4l2_dev, ntf);
> >> +}
> >> +
> >> +/***************************** platform device *******************************/
> >> +
> >> +static int rkisp1_register_platform_subdevs(struct rkisp1_device *dev)
> >> +{
> >> + int ret;
> >> +
> >> + ret = rkisp1_register_isp_subdev(dev, &dev->v4l2_dev);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + ret = rkisp1_register_stream_vdevs(dev);
> >> + if (ret < 0)
> >> + goto err_unreg_isp_subdev;
> >> +
> >> + ret = rkisp1_register_stats_vdev(&dev->stats_vdev, &dev->v4l2_dev, dev);
> >> + if (ret < 0)
> >> + goto err_unreg_stream_vdev;
> >> +
> >> + ret = rkisp1_register_params_vdev(&dev->params_vdev, &dev->v4l2_dev,
> >> + dev);
> >> + if (ret < 0)
> >> + goto err_unreg_stats_vdev;
> >> +
> >> + ret = isp_subdev_notifier(dev);
> >> + if (ret < 0) {
> >> + v4l2_err(&dev->v4l2_dev,
> >> + "Failed to register subdev notifier(%d)\n", ret);
> >> + goto err_unreg_params_vdev;
> >> + }
> >> +
> >> + return 0;
> >> +err_unreg_params_vdev:
> >> + rkisp1_unregister_params_vdev(&dev->params_vdev);
> >> +err_unreg_stats_vdev:
> >> + rkisp1_unregister_stats_vdev(&dev->stats_vdev);
> >> +err_unreg_stream_vdev:
> >> + rkisp1_unregister_stream_vdevs(dev);
> >> +err_unreg_isp_subdev:
> >> + rkisp1_unregister_isp_subdev(dev);
> >> + return ret;
> >> +}
> >> +
> >> +static const char * const rk3399_isp_clks[] = {
> >> + "clk_isp",
> >> + "aclk_isp",
> >> + "hclk_isp",
> >> + "aclk_isp_wrap",
> >> + "hclk_isp_wrap",
> >> +};
> >> +
> >> +static const char * const rk3288_isp_clks[] = {
> >> + "clk_isp",
> >> + "aclk_isp",
> >> + "hclk_isp",
> >> + "pclk_isp_in",
> >> + "sclk_isp_jpe",
> >> +};
> >> +
> >> +static const struct isp_match_data rk3288_isp_clk_data = {
> >> + .clks = rk3288_isp_clks,
> >> + .size = ARRAY_SIZE(rk3288_isp_clks),
> >> +};
> >> +
> >> +static const struct isp_match_data rk3399_isp_clk_data = {
> >> + .clks = rk3399_isp_clks,
> >> + .size = ARRAY_SIZE(rk3399_isp_clks),
> >> +};
> >> +
> >> +static const struct of_device_id rkisp1_plat_of_match[] = {
> >> + {
> >> + .compatible = "rockchip,rk3288-cif-isp",
> >> + .data = &rk3288_isp_clk_data,
> >> + }, {
> >> + .compatible = "rockchip,rk3399-cif-isp",
> >> + .data = &rk3399_isp_clk_data,
> >> + },
> >> + {},
> >> +};
> >> +MODULE_DEVICE_TABLE(of, rkisp1_plat_of_match);
> >> +
> >> +static irqreturn_t rkisp1_irq_handler(int irq, void *ctx)
> >> +{
> >> + struct device *dev = ctx;
> >> + struct rkisp1_device *rkisp1_dev = dev_get_drvdata(dev);
> >> + unsigned int mis_val;
> >> +
> >> + mis_val = readl(rkisp1_dev->base_addr + CIF_ISP_MIS);
> >> + if (mis_val)
> >> + rkisp1_isp_isr(mis_val, rkisp1_dev);
> >> +
> >> + mis_val = readl(rkisp1_dev->base_addr + CIF_MIPI_MIS);
> >> + if (mis_val)
> >> + rkisp1_mipi_isr(mis_val, rkisp1_dev);
> >> +
> >> + mis_val = readl(rkisp1_dev->base_addr + CIF_MI_MIS);
> >> + if (mis_val)
> >> + rkisp1_mi_isr(mis_val, rkisp1_dev);
> >> +
> >> + return IRQ_HANDLED;
> >> +}
> >> +
> >> +static int rkisp1_plat_probe(struct platform_device *pdev)
> >> +{
> >> + struct device_node *node = pdev->dev.of_node;
> >> + const struct isp_match_data *clk_data;
> >> + const struct of_device_id *match;
> >> + struct device *dev = &pdev->dev;
> >> + struct rkisp1_device *isp_dev;
> >> + struct v4l2_device *v4l2_dev;
> >> + unsigned int i;
> >> + int ret, irq;
> >> +
> >> + match = of_match_node(rkisp1_plat_of_match, node);
> >> + isp_dev = devm_kzalloc(dev, sizeof(*isp_dev), GFP_KERNEL);
> >> + if (!isp_dev)
> >> + return -ENOMEM;
> >> +
> >> + INIT_LIST_HEAD(&isp_dev->sensors);
> >> +
> >> + dev_set_drvdata(dev, isp_dev);
> >> + isp_dev->dev = dev;
> >> +
> >> + isp_dev->base_addr = devm_platform_ioremap_resource(pdev, 0);
> >> + if (IS_ERR(isp_dev->base_addr))
> >> + return PTR_ERR(isp_dev->base_addr);
> >> +
> >> + irq = platform_get_irq(pdev, 0);
> >> + if (irq < 0)
> >> + return irq;
> >> +
> >> + ret = devm_request_irq(dev, irq, rkisp1_irq_handler, IRQF_SHARED,
> >> + dev_driver_string(dev), dev);
> >> + if (ret < 0) {
> >> + dev_err(dev, "request irq failed: %d\n", ret);
> >> + return ret;
> >> + }
> >> +
> >> + isp_dev->irq = irq;
> >> + clk_data = match->data;
> >> +
> >> + for (i = 0; i < clk_data->size; i++)
> >> + isp_dev->clks[i].id = clk_data->clks[i];
> >> + ret = devm_clk_bulk_get(dev, clk_data->size, isp_dev->clks);
> >> + if (ret)
> >> + return ret;
> >> + isp_dev->clk_size = clk_data->size;
> >> +
> >> + atomic_set(&isp_dev->pipe.power_cnt, 0);
> >> + atomic_set(&isp_dev->pipe.stream_cnt, 0);
> >> + isp_dev->pipe.open = rkisp1_pipeline_open;
> >> + isp_dev->pipe.close = rkisp1_pipeline_close;
> >> + isp_dev->pipe.set_stream = rkisp1_pipeline_set_stream;
> >> +
> >> + rkisp1_stream_init(isp_dev, RKISP1_STREAM_SP);
> >> + rkisp1_stream_init(isp_dev, RKISP1_STREAM_MP);
> >> +
> >> + strscpy(isp_dev->media_dev.model, "rkisp1",
> >> + sizeof(isp_dev->media_dev.model));
> >> + isp_dev->media_dev.dev = &pdev->dev;
> >> + strscpy(isp_dev->media_dev.bus_info,
> >> + "platform: " DRIVER_NAME, sizeof(isp_dev->media_dev.bus_info));
> >> + media_device_init(&isp_dev->media_dev);
> >> +
> >> + v4l2_dev = &isp_dev->v4l2_dev;
> >> + v4l2_dev->mdev = &isp_dev->media_dev;
> >> + strscpy(v4l2_dev->name, "rkisp1", sizeof(v4l2_dev->name));
> >> + v4l2_ctrl_handler_init(&isp_dev->ctrl_handler, 5);
> >> + v4l2_dev->ctrl_handler = &isp_dev->ctrl_handler;
> >> +
> >> + ret = v4l2_device_register(isp_dev->dev, &isp_dev->v4l2_dev);
> >> + if (ret < 0)
> >
> > Once you've initialised the control handler, you'll need to free it in case
> > of an error. I.e. add one more label for that purpose near the end.
>
> control handler is not required, I'll remove it for the next version.
>
> >
> >> + return ret;
> >> +
> >> + ret = media_device_register(&isp_dev->media_dev);
> >> + if (ret < 0) {
> >> + v4l2_err(v4l2_dev, "Failed to register media device: %d\n",
> >> + ret);
> >> + goto err_unreg_v4l2_dev;
> >> + }
> >> +
> >> + /* create & register platefom subdev (from of_node) */
> >> + ret = rkisp1_register_platform_subdevs(isp_dev);
> >> + if (ret < 0)
> >> + goto err_unreg_media_dev;
> >> +
> >> + pm_runtime_enable(&pdev->dev);
> >> +
> >> + return 0;
> >> +
> >> +err_unreg_media_dev:
> >> + media_device_unregister(&isp_dev->media_dev);
> >> +err_unreg_v4l2_dev:
> >> + v4l2_device_unregister(&isp_dev->v4l2_dev);
> >> + return ret;
> >> +}
> >> +
> >> +static int rkisp1_plat_remove(struct platform_device *pdev)
> >> +{
> >> + struct rkisp1_device *isp_dev = platform_get_drvdata(pdev);
> >> +
> >> + pm_runtime_disable(&pdev->dev);
> >> + media_device_unregister(&isp_dev->media_dev);
> >> + v4l2_async_notifier_unregister(&isp_dev->notifier);
> >> + v4l2_async_notifier_cleanup(&isp_dev->notifier);
> >> + v4l2_device_unregister(&isp_dev->v4l2_dev);
> >> + rkisp1_unregister_params_vdev(&isp_dev->params_vdev);
> >> + rkisp1_unregister_stats_vdev(&isp_dev->stats_vdev);
> >> + rkisp1_unregister_stream_vdevs(isp_dev);
> >> + rkisp1_unregister_isp_subdev(isp_dev);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int __maybe_unused rkisp1_runtime_suspend(struct device *dev)
> >> +{
> >> + struct rkisp1_device *isp_dev = dev_get_drvdata(dev);
> >> +
> >> + clk_bulk_disable_unprepare(isp_dev->clk_size, isp_dev->clks);
> >> + return pinctrl_pm_select_sleep_state(dev);
> >> +}
> >> +
> >> +static int __maybe_unused rkisp1_runtime_resume(struct device *dev)
> >> +{
> >> + struct rkisp1_device *isp_dev = dev_get_drvdata(dev);
> >> + int ret;
> >> +
> >> + ret = pinctrl_pm_select_default_state(dev);
> >> + if (ret < 0)
> >> + return ret;
> >> + ret = clk_bulk_prepare_enable(isp_dev->clk_size, isp_dev->clks);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static const struct dev_pm_ops rkisp1_plat_pm_ops = {
> >> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> >> + pm_runtime_force_resume)
> >> + SET_RUNTIME_PM_OPS(rkisp1_runtime_suspend, rkisp1_runtime_resume, NULL)
> >> +};
> >> +
> >> +static struct platform_driver rkisp1_plat_drv = {
> >> + .driver = {
> >> + .name = DRIVER_NAME,
> >> + .of_match_table = of_match_ptr(rkisp1_plat_of_match),
> >> + .pm = &rkisp1_plat_pm_ops,
> >> + },
> >> + .probe = rkisp1_plat_probe,
> >> + .remove = rkisp1_plat_remove,
> >> +};
> >> +
> >> +module_platform_driver(rkisp1_plat_drv);
> >> +MODULE_AUTHOR("Rockchip Camera/ISP team");
> >> +MODULE_DESCRIPTION("Rockchip ISP1 platform driver");
> >> +MODULE_LICENSE("Dual BSD/GPL");
> >
> > BSD or MIT?
>
> Thanks for spotting this, I'll verify.
I think this was the only place where the BSD license was present. The file
header has MIT.
>
> >
> >> diff --git a/drivers/media/platform/rockchip/isp1/dev.h b/drivers/media/platform/rockchip/isp1/dev.h
> >> new file mode 100644
> >> index 000000000000..f7cbee316523
> >> --- /dev/null
> >> +++ b/drivers/media/platform/rockchip/isp1/dev.h
> >> @@ -0,0 +1,97 @@
> >> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> >> +/*
> >> + * Rockchip isp1 driver
> >> + *
> >> + * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
> >> + */
> >> +
> >> +#ifndef _RKISP1_DEV_H
> >> +#define _RKISP1_DEV_H
> >> +
> >> +#include <linux/clk.h>
> >> +
> >> +#include "capture.h"
> >> +#include "rkisp1.h"
> >> +#include "isp_params.h"
> >> +#include "isp_stats.h"
> >> +
> >> +#define DRIVER_NAME "rkisp1"
> >> +#define ISP_VDEV_NAME DRIVER_NAME "_ispdev"
> >> +#define SP_VDEV_NAME DRIVER_NAME "_selfpath"
> >> +#define MP_VDEV_NAME DRIVER_NAME "_mainpath"
> >> +#define DMA_VDEV_NAME DRIVER_NAME "_dmapath"
> >> +
> >> +#define GRP_ID_SENSOR BIT(0)
> >> +#define GRP_ID_MIPIPHY BIT(1)
> >> +#define GRP_ID_ISP BIT(2)
> >> +#define GRP_ID_ISP_MP BIT(3)
> >> +#define GRP_ID_ISP_SP BIT(4)
> >> +
> >> +#define RKISP1_MAX_BUS_CLK 8
> >> +#define RKISP1_MAX_SENSOR 2
> >> +#define RKISP1_MAX_PIPELINE 4
> >> +
> >> +/*
> >> + * struct rkisp1_pipeline - An ISP hardware pipeline
> >> + *
> >> + * Capture device call other devices via pipeline
> >> + *
> >> + * @num_subdevs: number of linked subdevs
> >> + * @power_cnt: pipeline power count
> >> + * @stream_cnt: stream power count
> >> + */
> >> +struct rkisp1_pipeline {
> >> + struct media_pipeline pipe;
> >> + int num_subdevs;
> >> + atomic_t power_cnt;
> >> + atomic_t stream_cnt;
> >> + struct v4l2_subdev *subdevs[RKISP1_MAX_PIPELINE];
> >> + int (*open)(struct rkisp1_pipeline *p,
> >> + struct media_entity *me, bool prepare);
> >> + int (*close)(struct rkisp1_pipeline *p);
> >> + int (*set_stream)(struct rkisp1_pipeline *p, bool on);
> >> +};
> >> +
> >> +/*
> >> + * struct rkisp1_sensor - Sensor information
> >> + * @mbus: media bus configuration
> >> + */
> >> +struct rkisp1_sensor {
> >> + struct v4l2_subdev *sd;
> >> + struct v4l2_mbus_config mbus;
> >> + unsigned int lanes;
> >> + struct phy *dphy;
> >> + struct list_head list;
> >> +};
> >
> > You seem to also have struct sensor_async_subdev that appears to contain
> > similar information. Would it be possible to unify the two?
> >
> > This would appear to allow you getting rid of functions such as
> > sd_to_sensor, for instance.
>
> ack, I managed to get rid of this, and I don't even need to keep them
> on a list, I'll submit in the next version.
Nice!
>
> Thanks a lot for your review
You're welcome!
--
Kind regards,
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply
* Re: [PATCH v2 05/15] net: phy: adin: add {write,read}_mmd hooks
From: Ardelean, Alexandru @ 2019-08-09 12:05 UTC (permalink / raw)
To: andrew@lunn.ch
Cc: davem@davemloft.net, hkallweit1@gmail.com,
devicetree@vger.kernel.org, mark.rutland@arm.com,
linux-kernel@vger.kernel.org, f.fainelli@gmail.com,
netdev@vger.kernel.org, robh+dt@kernel.org
In-Reply-To: <20190808153514.GE27917@lunn.ch>
On Thu, 2019-08-08 at 17:35 +0200, Andrew Lunn wrote:
> [External]
>
> On Thu, Aug 08, 2019 at 03:30:16PM +0300, Alexandru Ardelean wrote:
> > Both ADIN1200 & ADIN1300 support Clause 45 access.
> > The Extended Management Interface (EMI) registers are accessible via both
> > Clause 45 (at register MDIO_MMD_VEND1) and using Clause 22.
> >
> > However, the Clause 22 MMD access operations differ from the implementation
> > in the kernel, in the sense that it uses registers ExtRegPtr (0x10) &
> > ExtRegData (0x11) to access Clause 45 & EMI registers.
>
> It is not that they differ from what the kernel supports. Its that
> they differ from what the Standard says they should use. These
> registers are defined in 802.3, part of C22, and this hardware
> implements the standard incorrectly.
Will update comment.
I did not find a document describing this as a standard.
But, I admit I am terrible when finding some docs.
I only found this presentation from a while back:
http://www.ieee802.org/3/efm/public/nov02/oam/pannell_oam_1_1102.pdf
It seemed more like a proposal.
Thanks
Alex
>
> Andrew
^ permalink raw reply
* Re: [Patch V6 7/8] usb: gadget: Add UDC driver for tegra XUSB device mode controller
From: Felipe Balbi @ 2019-08-09 12:03 UTC (permalink / raw)
To: Nagarjuna Kristam, gregkh, thierry.reding, jonathanh,
mark.rutland, robh+dt
Cc: devicetree, linux-tegra, linux-usb
In-Reply-To: <1565257046-9890-8-git-send-email-nkristam@nvidia.com>
Hi,
Nagarjuna Kristam <nkristam@nvidia.com> writes:
> This patch adds UDC driver for tegra XUSB 3.0 device mode controller.
> XUSB device mode controller supports SS, HS and FS modes
>
> Based on work by:
> Mark Kuo <mkuo@nvidia.com>
> Hui Fu <hfu@nvidia.com>
> Andrew Bresticker <abrestic@chromium.org>
>
> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
> Acked-by: Thierry Reding <treding@nvidia.com>
> ---
> drivers/usb/gadget/udc/Kconfig | 11 +
> drivers/usb/gadget/udc/Makefile | 1 +
> drivers/usb/gadget/udc/tegra_xudc.c | 3808 +++++++++++++++++++++++++++++++++++
> 3 files changed, 3820 insertions(+)
> create mode 100644 drivers/usb/gadget/udc/tegra_xudc.c
>
> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
> index ef0259a..fe6028e 100644
> --- a/drivers/usb/gadget/udc/Kconfig
> +++ b/drivers/usb/gadget/udc/Kconfig
> @@ -440,6 +440,17 @@ config USB_GADGET_XILINX
> dynamically linked module called "udc-xilinx" and force all
> gadget drivers to also be dynamically linked.
>
> +config USB_TEGRA_XUDC
> + tristate "NVIDIA Tegra Superspeed USB 3.0 Device Controller"
> + depends on ARCH_TEGRA
I need at least a COMPILE_TEST here.
--
balbi
^ permalink raw reply
* Re: [PATCH v2 02/15] net: phy: adin: hook genphy_read_abilities() to get_features
From: Ardelean, Alexandru @ 2019-08-09 12:00 UTC (permalink / raw)
To: hkallweit1@gmail.com, andrew@lunn.ch
Cc: f.fainelli@gmail.com, mark.rutland@arm.com,
devicetree@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, robh+dt@kernel.org,
davem@davemloft.net
In-Reply-To: <eeda87c9-bdba-8ef7-6043-85a16bd2cfc2@gmail.com>
On Thu, 2019-08-08 at 21:32 +0200, Heiner Kallweit wrote:
> [External]
>
> On 08.08.2019 17:24, Andrew Lunn wrote:
> > On Thu, Aug 08, 2019 at 03:30:13PM +0300, Alexandru Ardelean wrote:
> > > The ADIN PHYs can operate with Clause 45, however they are not typical for
> > > how phylib considers Clause 45 PHYs.
> > >
> > > If the `features` field & the `get_features` hook are unspecified, and the
> > > device wants to operate via Clause 45, it would also try to read features
> > > via the `genphy_c45_pma_read_abilities()`, which will try to read PMA regs
> > > that are unsupported.
> > >
> > > Hooking the `genphy_read_abilities()` function to the `get_features` hook
> > > will ensure that this does not happen and the PHY features are read
> > > correctly regardless of Clause 22 or Clause 45 operation.
> >
> > I think we need to stop and think about a PHY which supports both C22
> > and C45.
> >
> > How does bus enumeration work? Is it discovered twice? I've always
> > considered phydev->is_c45 means everything is c45, not that some
> > registers can be accessed via c45. But the driver is mixing c22 and
> > c45. Does the driver actually require c45? Are some features which are
> > only accessibly via C45? What does C45 actually bring us for this
> > device?
> >
Hmm, I can't answer [all] these questions.
These PHYs seem to be a bit different from the rest that I looked at in drivers/net/phy with regard to C45 & C22.
And C45 seems to be more/closer related to 10G PHYs [from what I can tell].
The ADIN PHYs can operate only in C22.
The only thing that is needed [and a bit special] is EEE, which [for C22] requires the translation layer to convert C45
reg addresses to internal C22 equivalent.
> genphy_c45_pma_read_abilities() is only called if phydev->is_c45 is set.
> And this flag means that the PHY complies with Clause 45 incl. all the
> standard devices like PMA. In the case here only some vendor-specific
> registers can be accessed via Clause 45 and therefore is_c45 shouldn't
> bet set. As a consequence this patch isn't needed.
ack, will drop patch
>
> > Andrew
> >
> Heiner
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox