* Re: [PATCH 9/9] ARM: dts: silk: Drop MTD partitioning from DT
From: Wolfram Sang @ 2018-05-22 12:34 UTC (permalink / raw)
To: Marek Vasut
Cc: devicetree, Geert Uytterhoeven, linux-renesas-soc,
Laurent Pinchart, Simon Horman, linux-arm-kernel, Marek Vasut
In-Reply-To: <20180522120257.13232-9-marek.vasut+renesas@gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 814 bytes --]
On Tue, May 22, 2018 at 02:02:57PM +0200, Marek Vasut wrote:
> Drop the MTD partitioning from DT, since it does not describe HW
> and to give way to a more flexible kernel command line partition
> passing.
>
> To retain the original partitioning, assure you have enabled
> CONFIG_MTD_CMDLINE_PARTS in your kernel config and add the
> following to your kernel command line:
>
> mtdparts=spi0.0:256k@0(loader),4096k(user),-(flash)
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Simon Horman <horms+renesas@verge.net.au>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: linux-renesas-soc@vger.kernel.org
Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] iommu/ipmmu-vmsa: Document R-Car V3H and E3 IPMMU DT bindings
From: Simon Horman @ 2018-05-22 12:50 UTC (permalink / raw)
To: Magnus Damm
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw,
geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <152691369384.29456.13581918319106400529.sendpatchset@little-apple>
On Mon, May 21, 2018 at 11:41:33PM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm+renesas-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>
>
> Update the IPMMU DT binding documentation to include the compat strings
> for the IPMMU devices included in the R-Car V3H and E3 SoCs.
>
> Signed-off-by: Magnus Damm <damm+renesas-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>
Reviewed-by: Simon Horman <horms+renesas-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
> ---
>
> Developed on top of renesas-drivers-2018-05-15-v4.17-rc5
>
> Documentation/devicetree/bindings/iommu/renesas,ipmmu-vmsa.txt | 2 ++
> 1 file changed, 2 insertions(+)
>
> --- 0001/Documentation/devicetree/bindings/iommu/renesas,ipmmu-vmsa.txt
> +++ work/Documentation/devicetree/bindings/iommu/renesas,ipmmu-vmsa.txt 2018-05-21 23:37:16.370607110 +0900
> @@ -20,6 +20,8 @@ Required Properties:
> - "renesas,ipmmu-r8a7795" for the R8A7795 (R-Car H3) IPMMU.
> - "renesas,ipmmu-r8a7796" for the R8A7796 (R-Car M3-W) IPMMU.
> - "renesas,ipmmu-r8a77970" for the R8A77970 (R-Car V3M) IPMMU.
> + - "renesas,ipmmu-r8a77980" for the R8A77980 (R-Car V3H) IPMMU.
> + - "renesas,ipmmu-r8a77990" for the R8A77990 (R-Car E3) IPMMU.
> - "renesas,ipmmu-r8a77995" for the R8A77995 (R-Car D3) IPMMU.
> - "renesas,ipmmu-vmsa" for generic R-Car Gen2 or RZ/G1 VMSA-compatible
> IPMMU.
>
^ permalink raw reply
* Re: [PATCH] cpufreq: Add Kryo CPU scaling driver
From: Sudeep Holla @ 2018-05-22 13:07 UTC (permalink / raw)
To: Ilia Lin
Cc: viresh.kumar, linux-clk, devicetree, linux-kernel, linux-pm,
linux-arm-msm, linux-soc, linux-arm-kernel, Sudeep Holla
In-Reply-To: <1526988585-21678-1-git-send-email-ilialin@codeaurora.org>
On Tue, May 22, 2018 at 02:29:45PM +0300, Ilia Lin wrote:
> In Certain QCOM SoCs like apq8096 and msm8996 that have KRYO processors,
> the CPU frequency subset and voltage value of each OPP varies
> based on the silicon variant in use. Qualcomm Process Voltage Scaling Tables
> defines the voltage and frequency value based on the msm-id in SMEM
> and speedbin blown in the efuse combination.
> The qcom-cpufreq-kryo driver reads the msm-id and efuse value from the SoC
> to provide the OPP framework with required information.
> This is used to determine the voltage and frequency value for each OPP of
> operating-points-v2 table when it is parsed by the OPP framework.
>
> Signed-off-by: Ilia Lin <ilialin@codeaurora.org>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
[...]
> +
> + switch (msm8996_version) {
> + case MSM8996_V3:
> + versions = 1 << (unsigned int)(*speedbin);
> + break;
> + case MSM8996_SG:
> + versions = 1 << ((unsigned int)(*speedbin) + 4);
> + break;
> + default:
> + BUG();
> + break;
> + }
> +
> + for_each_possible_cpu(cpu) {
> + cpu_dev = get_cpu_device(cpu);
> + if (NULL == cpu_dev) {
> + ret = -ENODEV;
> + goto free_opp;
> + }
> +
> + opp_tables[cpu] = dev_pm_opp_set_supported_hw(cpu_dev,
> + &versions, 1);
Will be not get NULL for all CPUs except 0 ?
I haven't seen the patches from Viresh yet, if that prevents getting NULL
or not.
--
Regards,
Sudeep
^ permalink raw reply
* RE: [patch v21 1/4] drivers: jtag: Add JTAG core driver
From: Oleksandr Shamray @ 2018-05-22 13:08 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Greg Kroah-Hartman, Arnd Bergmann, Linux Kernel Mailing List,
linux-arm Mailing List, devicetree, openbmc@lists.ozlabs.org,
Joel Stanley, Jiří Pírko, Tobias Klauser,
open list:SERIAL DRIVERS, Vadim Pasternak, system-sw-low-level,
Rob Herring, openocd-devel-owner@lists.sourceforge.net,
linux-api@vger.kernel.org, David S. Miller
[-- Attachment #1: Type: text/plain, Size: 7740 bytes --]
Hi Andy.
Thanks for review.
Please read my answers inline.
> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: 16 мая 2018 г. 0:22
> To: Oleksandr Shamray <oleksandrs@mellanox.com<mailto:oleksandrs@mellanox.com>>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org<mailto:gregkh@linuxfoundation.org>>; Arnd Bergmann
> <arnd@arndb.de<mailto:arnd@arndb.de>>; Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org<mailto:linux-kernel@vger.kernel.org>>; linux-arm Mailing List
> <linux-arm-kernel@lists.infradead.org<mailto:linux-arm-kernel@lists.infradead.org>>; devicetree
> <devicetree@vger.kernel.org<mailto:devicetree@vger.kernel.org>>; openbmc@lists.ozlabs.org<mailto:openbmc@lists.ozlabs.org>; Joel Stanley
> <joel@jms.id.au<mailto:joel@jms.id.au>>; Jiří Pírko <jiri@resnulli.us<mailto:jiri@resnulli.us>>; Tobias Klauser
> <tklauser@distanz.ch<mailto:tklauser@distanz.ch>>; open list:SERIAL DRIVERS <linux-
> serial@vger.kernel.org<mailto:serial@vger.kernel.org>>; Vadim Pasternak <vadimp@mellanox.com<mailto:vadimp@mellanox.com>>;
> system-sw-low-level <system-sw-low-level@mellanox.com<mailto:system-sw-low-level@mellanox.com>>; Rob Herring
> <robh+dt@kernel.org<mailto:robh+dt@kernel.org>>; openocd-devel-owner@lists.sourceforge.net<mailto:openocd-devel-owner@lists.sourceforge.net>;
> linux- api@vger.kernel.org<mailto:api@vger.kernel.org>; David S. Miller <davem@davemloft.net<mailto:davem@davemloft.net>>;
> Mauro Carvalho Chehab <mchehab@kernel.org<mailto:mchehab@kernel.org>>; Jiri Pirko
> <jiri@mellanox.com<mailto:jiri@mellanox.com>>
> Subject: Re: [patch v21 1/4] drivers: jtag: Add JTAG core driver
>
> On Tue, May 15, 2018 at 5:21 PM, Oleksandr Shamray
> <oleksandrs@mellanox.com<mailto:oleksandrs@mellanox.com>> wrote:
> > Initial patch for JTAG driver
> > JTAG class driver provide infrastructure to support
> > hardware/software JTAG platform drivers. It provide user layer API
> > interface for flashing and debugging external devices which equipped
> > with JTAG interface using standard transactions.
> >
> > Driver exposes set of IOCTL to user space for:
> > - XFER:
> > - SIR (Scan Instruction Register, IEEE 1149.1 Data Register scan);
> > - SDR (Scan Data Register, IEEE 1149.1 Instruction Register scan);
> > - RUNTEST (Forces the IEEE 1149.1 bus to a run state for a specified
> > number of clocks).
> > - SIOCFREQ/GIOCFREQ for setting and reading JTAG frequency.
> >
> > Driver core provides set of internal APIs for allocation and
> > registration:
> > - jtag_register;
> > - jtag_unregister;
> > - jtag_alloc;
> > - jtag_free;
> >
> > Platform driver on registration with jtag-core creates the next
> > entry in dev folder:
> > /dev/jtagX
>
> > 0xB0 all RATIO devices in development:
> > <mailto:vgo@ratio.de>
> > 0xB1 00-1F PPPoX <mailto:mostrows@styx.uwaterloo.ca>
> > +0xB2 00-0f linux/jtag.h JTAG driver
> > +
> > +<mailto:oleksandrs@mellanox.com>
>
> Consider to preserve style (upper vs. lower).
JTAG in code is lower (jtag) cane but in descriptions and notes it is upper (JTAG).
In all places which do not correspond to this I will fix.
>
> > + This provides basic core functionality support for JTAG class devices.
> > + Hardware that is equipped with a JTAG microcontroller can be
> > + supported by using this driver's interfaces.
> > + This driver exposes a set of IOCTLs to the user space for
> > + the following commands:
> > + SDR: (Scan Data Register) Performs an IEEE 1149.1 Data Register scan
> > + SIR: (Scan Instruction Register) Performs an IEEE 1149.1 Instruction
> > + Register scan.
> > + RUNTEST: Forces the IEEE 1149.1 bus to a run state for a specified
> > + number of clocks or a specified time period.
>
> Something feels wrong with formatting here.
>
Will fix
> > +#define MAX_JTAG_NAME_LEN (sizeof("jtag") + 5)
>
> Interesting definition. Why not to set to 10 explicitly? And why 10?
> (16 sounds better)
>
5 - is a max len of JTAG device id in device name. I will add define to it.
In general I don't see the case for the system with hundreds JTAG interfaces.
I will limit maximum jtag master to 255 devices and change id len to 3
#define MAX_JTAG_ID_STR_LEN 5
#define MAX_JTAG_NAME_LEN (sizeof("jtag") + MAX_JTAG_ID_STR_LEN)
> > +struct jtag {
> > + struct miscdevice miscdev;
>
> > + struct device *dev;
>
> Doesn't miscdev parent contain exactly this one?
Yes.
Will fix.
>
> > + const struct jtag_ops *ops;
> > + int id;
> > + bool opened;
> > + struct mutex open_lock;
> > + unsigned long priv[0];
> > +};
>
> > + err = copy_to_user(u64_to_user_ptr(xfer.tdio),
> > + (void *)(xfer_data), data_size);
>
> Redundant parens in one case. Check the rest similar places.
>
Yes.
> > +static int jtag_open(struct inode *inode, struct file *file) {
>
> > + struct jtag *jtag = container_of(file->private_data, struct jtag,
> > + miscdev);
>
> I would don't care about length and put it on one line.
>
But following to LINUX kernel style, it should be no longer than 80 symbols.
It will not pass by ./scripts/checkpatch.pl
Will it be OK to send a patch which failed 80 symbols limitation check?
> > + if (jtag->opened) {
> > + jtag->opened = true;
> > + jtag->opened = false;
>
> Can it be opened non exclusively several times? If so, this needs to
> be a ref counter instead.
It can be opened only once.
>
> > + if (!ops->idle || !ops->mode_set || !ops->status_get || !ops->xfer)
> > + return NULL;
>
> Are all of them mandatory?
>
Yes, except "mode_set"
Will remove mode_set from check
> > +int jtag_register(struct jtag *jtag)
>
> Perhaps devm_ variant.
Jtag driver uses miscdevice and related misc_register and misc_deregister calls for creation and destruction. There is no device object prior to call to misc_register, which could be used in devm_jtag_register.
>
> > +#define jtag_u64_to_ptr(arg) ((void *)(uintptr_t)arg)
>
Redundant. Removed.
> Where is this used or supposed to be used?
>
> > +#define JTAG_MAX_XFER_DATA_LEN 65535
>
> Is this limitation from some spec?
> Otherwise why not to allow 64K?
>
It not limited by specification.
But we enforce an upper bound for the length here, to prevent users from draining kernel memory with giant buffers.
> > +/**
> > + * struct jtag_ops - callbacks for jtag control functions:
> > + *
> > + * @freq_get: get frequency function. Filled by device driver
> > + * @freq_set: set frequency function. Filled by device driver
> > + * @status_get: set status function. Filled by device driver
> > + * @idle: set JTAG to idle state function. Filled by device driver
> > + * @xfer: send JTAG xfer function. Filled by device driver */
>
> Perhaps you need to describe which of them are _really_ mandatory and
> which are optional.
>
Ok
> --
> With Best Regards,
> Andy Shevchenko
Best Regards,
Oleksandr Shamray
[-- Attachment #2: Type: text/html, Size: 25872 bytes --]
^ permalink raw reply
* RE: [patch v21 2/4] drivers: jtag: Add Aspeed SoC 24xx and 25xx families JTAG master driver
From: Oleksandr Shamray @ 2018-05-22 13:08 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Greg Kroah-Hartman, Arnd Bergmann, Linux Kernel Mailing List,
linux-arm Mailing List, devicetree, openbmc@lists.ozlabs.org,
Joel Stanley, Jiří Pírko, Tobias Klauser,
open list:SERIAL DRIVERS, Vadim Pasternak, system-sw-low-level,
Rob Herring, openocd-devel-owner@lists.sourceforge.net,
linux-api@vger.kernel.org, David S. Miller
[-- Attachment #1: Type: text/plain, Size: 12382 bytes --]
Hi Vadim.
Please check my answer to Andy.
Hi Andy.
Thanks for review.
Please read my answers inline.
> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: 16 мая 2018 г. 0:00
> To: Oleksandr Shamray <oleksandrs@mellanox.com<mailto:oleksandrs@mellanox.com>>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org<mailto:gregkh@linuxfoundation.org>>; Arnd Bergmann
> <arnd@arndb.de<mailto:arnd@arndb.de>>; Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org<mailto:linux-kernel@vger.kernel.org>>; linux-arm Mailing List
> <linux-arm-kernel@lists.infradead.org<mailto:linux-arm-kernel@lists.infradead.org>>; devicetree
> <devicetree@vger.kernel.org<mailto:devicetree@vger.kernel.org>>; openbmc@lists.ozlabs.org<mailto:openbmc@lists.ozlabs.org>; Joel Stanley
> <joel@jms.id.au<mailto:joel@jms.id.au>>; Jiří Pírko <jiri@resnulli.us<mailto:jiri@resnulli.us>>; Tobias Klauser
> <tklauser@distanz.ch<mailto:tklauser@distanz.ch>>; open list:SERIAL DRIVERS <linux-
> serial@vger.kernel.org<mailto:serial@vger.kernel.org>>; Vadim Pasternak <vadimp@mellanox.com<mailto:vadimp@mellanox.com>>;
> system-sw-low-level <system-sw-low-level@mellanox.com<mailto:system-sw-low-level@mellanox.com>>; Rob Herring
> <robh+dt@kernel.org<mailto:robh+dt@kernel.org>>; openocd-devel-owner@lists.sourceforge.net<mailto:openocd-devel-owner@lists.sourceforge.net>;
> linux- api@vger.kernel.org<mailto:api@vger.kernel.org>; David S. Miller <davem@davemloft.net<mailto:davem@davemloft.net>>;
> Mauro Carvalho Chehab <mchehab@kernel.org<mailto:mchehab@kernel.org>>; Jiri Pirko
> <jiri@mellanox.com<mailto:jiri@mellanox.com>>
> Subject: Re: [patch v21 2/4] drivers: jtag: Add Aspeed SoC 24xx and
> 25xx families JTAG master driver
>
> On Tue, May 15, 2018 at 5:21 PM, Oleksandr Shamray
> <oleksandrs@mellanox.com<mailto:oleksandrs@mellanox.com>> wrote:
> > Driver adds support of Aspeed 2500/2400 series SOC JTAG master
> controller.
> >
> > Driver implements the following jtag ops:
> > - freq_get;
> > - freq_set;
> > - status_get;
> > - idle;
> > - xfer;
> >
> > It has been tested on Mellanox system with BMC equipped with Aspeed
> > 2520 SoC for programming CPLD devices.
>
> > +#define ASPEED_JTAG_DATA 0x00
> > +#define ASPEED_JTAG_INST 0x04
> > +#define ASPEED_JTAG_CTRL 0x08
> > +#define ASPEED_JTAG_ISR 0x0C
> > +#define ASPEED_JTAG_SW 0x10
> > +#define ASPEED_JTAG_TCK 0x14
> > +#define ASPEED_JTAG_EC 0x18
> > +
> > +#define ASPEED_JTAG_DATA_MSB 0x01
> > +#define ASPEED_JTAG_DATA_CHUNK_SIZE 0x20
>
>
> > +#define ASPEED_JTAG_IOUT_LEN(len) (ASPEED_JTAG_CTL_ENG_EN |\
> > + ASPEED_JTAG_CTL_ENG_OUT_EN
> > +|\
> > +
> > +ASPEED_JTAG_CTL_INST_LEN(len))
>
> Better to read
>
> #define MY_COOL_CONST_OR_MACRO(xxx) \
> ...
>
> > +#define ASPEED_JTAG_DOUT_LEN(len) (ASPEED_JTAG_CTL_ENG_EN
> |\
> > + ASPEED_JTAG_CTL_ENG_OUT_EN
> > + |\
> > +
> > +ASPEED_JTAG_CTL_DATA_LEN(len))
>
> Ditto.
Ok. Changed to:
#define ASPEED_JTAG_IOUT_LEN(len) \
(ASPEED_JTAG_CTL_ENG_EN | \
ASPEED_JTAG_CTL_ENG_OUT_EN | \
ASPEED_JTAG_CTL_INST_LEN(len))
#define ASPEED_JTAG_DOUT_LEN(len) \
(ASPEED_JTAG_CTL_ENG_EN | \
ASPEED_JTAG_CTL_ENG_OUT_EN | \
ASPEED_JTAG_CTL_DATA_LEN(len))
>
> > +static char *end_status_str[] = {"idle", "ir pause", "drpause"};
>
> > +static int aspeed_jtag_freq_set(struct jtag *jtag, u32 freq) {
> > + struct aspeed_jtag *aspeed_jtag = jtag_priv(jtag);
> > + unsigned long apb_frq;
> > + u32 tck_val;
> > + u16 div;
> > +
> > + apb_frq = clk_get_rate(aspeed_jtag->pclk);
>
> > + div = (apb_frq % freq == 0) ? (apb_frq / freq) - 1 :
> > + (apb_frq / freq);
>
> Isn't it the same as
>
> div = (apb_frq - 1) / freq;
>
> ?
>
Seems it is same. Thanks.
> > + tck_val = aspeed_jtag_read(aspeed_jtag, ASPEED_JTAG_TCK);
> > + aspeed_jtag_write(aspeed_jtag,
> > + (tck_val & ASPEED_JTAG_TCK_DIVISOR_MASK) | div,
> > + ASPEED_JTAG_TCK);
> > + return 0;
> > +}
>
> > +static void aspeed_jtag_sw_delay(struct aspeed_jtag *aspeed_jtag,
> > +int
> > +cnt) {
> > + int i;
> > +
> > + for (i = 0; i < cnt; i++)
> > + aspeed_jtag_read(aspeed_jtag, ASPEED_JTAG_SW);
>
> Isn't it readsl() (or how it's called, I don't remember).
>
No, readsl reads data into buffer. But in this place read used for make software delay.
Aspeed jtag driver supports 2 modes:
1 - hw mode with the hardware controlled JTAG states
and pins
2 - with software controlled pins.
This part of code used in sw-mode and generates delay for JTAG bit-bang .
I will change it to ndelay().
> > +}
>
> > +static void aspeed_jtag_wait_instruction_pause(struct aspeed_jtag
> > +*aspeed_jtag) {
> > + wait_event_interruptible(aspeed_jtag->jtag_wq, aspeed_jtag->flag &
> > + ASPEED_JTAG_ISR_INST_PAUSE);
>
> In such cases I prefer to see a new line with a parameter in full.
> Check all places.
Full parameter doesn't fit into max 80 symbols per line limitation.
But following to LINUX kernel style, it should be no longer than 80 symbols.
It will not pass by ./scripts/checkpatch.pl
What do you think about this?
>
> > + aspeed_jtag->flag &= ~ASPEED_JTAG_ISR_INST_PAUSE; }
>
> > +static void aspeed_jtag_sm_cycle(struct aspeed_jtag *aspeed_jtag,
> > +const
> u8 *tms,
> > + int len) {
> > + int i;
> > +
> > + for (i = 0; i < len; i++)
> > + aspeed_jtag_tck_cycle(aspeed_jtag, tms[i], 0); }
> > +
> > +static void aspeed_jtag_run_test_idle_sw(struct aspeed_jtag
> *aspeed_jtag,
> > + struct jtag_run_test_idle
> > +*runtest) {
> > + static const u8 sm_pause_irpause[] = {1, 1, 1, 1, 0, 1, 0};
> > + static const u8 sm_pause_drpause[] = {1, 1, 1, 0, 1, 0};
> > + static const u8 sm_idle_irpause[] = {1, 1, 0, 1, 0};
> > + static const u8 sm_idle_drpause[] = {1, 0, 1, 0};
> > + static const u8 sm_pause_idle[] = {1, 1, 0};
> > + int i;
> > +
> > + /* SW mode from idle/pause-> to pause/idle */
> > + if (runtest->reset) {
> > + for (i = 0; i < ASPEED_JTAG_RESET_CNTR; i++)
> > + aspeed_jtag_tck_cycle(aspeed_jtag, 1, 0);
> > + }
>
> I would rather split this big switch to a few helper functions per
> each status of surrounding switch.
>
Ok.
Will do it.
> > +
> > + /* Stay on IDLE for at least TCK cycle */
> > + for (i = 0; i < runtest->tck; i++)
> > + aspeed_jtag_tck_cycle(aspeed_jtag, 0, 0); }
>
>
> > +/**
> > + * aspeed_jtag_run_test_idle:
> > + * JTAG reset: generates at least 9 TMS high and 1 TMS low to force
> > + * devices into Run-Test/Idle State.
> > + */
>
> It's rather broken kernel doc.
Deleted.
>
> > + aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_CTL_ENG_EN |
> > + ASPEED_JTAG_CTL_ENG_OUT_EN |
> > + ASPEED_JTAG_CTL_FORCE_TMS,
> > + ASPEED_JTAG_CTRL);
>
> > + aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_EC_GO_IDLE,
> > + ASPEED_JTAG_EC);
>
> > + aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_SW_MODE_EN |
> > + ASPEED_JTAG_SW_MODE_TDIO, ASPEED_JTAG_SW);
>
> Here you have permutations of flag some of which are repeatetive in
> the code. Perhaps make additional definitions instead.
> Check other similar places.
>
Ok. Will add defined for repeated flags
>
> > + char tdo;
>
> Indentation.
Ok.
>
> > + if (xfer->direction == JTAG_READ_XFER)
> > + tdi = UINT_MAX;
> > + else
> > + tdi = data[index];
>
> > + if (xfer->direction == JTAG_READ_XFER)
> > + tdi = UINT_MAX;
> > + else
> > + tdi = data[index];
>
> Take your time to think how the above duplication can be avoided.
>
I can't avoid this but it can changed to define:
#define ASPEED_JTAG_GET_TDI(direction, data) \
(direction == JTAG_READ_XFER) ? UNIT_MAX : data; And use as tdi = ASPEED_JTAG_GET_TDI(xfer->direction, data[index]);
> > + }
> > + }
> > +
> > + tdo = aspeed_jtag_tck_cycle(aspeed_jtag, 1, tdi &
> ASPEED_JTAG_DATA_MSB);
> > + data[index] |= tdo << (shift_bits %
> > +ASPEED_JTAG_DATA_CHUNK_SIZE); }
>
>
> > + if (endstate != JTAG_STATE_IDLE) {
>
> Why not to use positive check?
>
Will restructure to have positive check
if (endstate == JTAG_STATE_IDLE) {
...
} else {
...
}
> > + int i;
> > +
> > + for (i = 0; i <= xfer->length / BITS_PER_BYTE; i++) {
> > + pos += snprintf(&dbg_str[pos], sizeof(dbg_str) - pos,
> > + "0x%02x ", xfer_data[i]);
> > + }
>
> Oh, NO! Consider reading printk-formats (for %*ph) and other
> documentation about available APIs.
Ok.
>
> > + if (!(aspeed_jtag->mode & JTAG_XFER_HW_MODE)) {
> > + /* SW mode */
>
> This is rather too complex to be in one function.
>
Will split to separate functions.
> > + } else {
>
> > + /* hw mode */
> > + aspeed_jtag_write(aspeed_jtag, 0, ASPEED_JTAG_SW);
> > + aspeed_jtag_xfer_hw(aspeed_jtag, xfer, data);
>
> For symmetry it might be another function.
>
> > + }
>
> > + dev_dbg(aspeed_jtag->dev, "status %x\n", status);
>
> Perhaps someone should become familiar with tracepoints?
>
> > + dev_err(aspeed_jtag->dev, "irq status:%x\n",
> > + status);
>
>
> Huh, really?! SPAM.
I will review and delete redundant debug messages.
>
> (I would drop it completely, though you may use ratelimited variant)
>
> > + ret = IRQ_NONE;
> > + }
> > + return ret;
> > +}
>
> > + clk_prepare_enable(aspeed_jtag->pclk);
>
> This might fail.
Will add error check
>
> > + dev_dbg(&pdev->dev, "IRQ %d.\n", aspeed_jtag->irq);
>
> Noise even for debug.
Agree.
>
> > + err = jtag_register(jtag);
>
> Perhaps we might have devm_ variant of this. Check how SPI framework
> deal with a such.
>
Jtag driver uses miscdevice and related misc_register and misc_deregister calls for creation and destruction. There is no device object prior to call to misc_register, which could be used in devm_jtag_register.
> > +static int aspeed_jtag_remove(struct platform_device *pdev) {
>
> > + struct jtag *jtag;
> > +
> > + jtag = platform_get_drvdata(pdev);
>
> Usually we put this on one line
+
>
> > + aspeed_jtag_deinit(pdev, jtag_priv(jtag));
> > + jtag_unregister(jtag);
> > + jtag_free(jtag);
> > + return 0;
> > +}
>
>
> --
> With Best Regards,
> Andy Shevchenko
Best Regards,
Oleksandr Shamray
[-- Attachment #2: Type: text/html, Size: 48134 bytes --]
^ permalink raw reply
* RE: RE: [patch v21 1/4] drivers: jtag: Add JTAG core driver
From: Vadim Pasternak @ 2018-05-22 13:18 UTC (permalink / raw)
To: Oleksandr Shamray, Andy Shevchenko
Cc: Greg Kroah-Hartman, Arnd Bergmann, Linux Kernel Mailing List,
linux-arm Mailing List, devicetree, openbmc@lists.ozlabs.org,
Joel Stanley, Jiří Pírko, Tobias Klauser,
open list:SERIAL DRIVERS, system-sw-low-level, Rob Herring,
openocd-devel-owner@lists.sourceforge.net,
linux-api@vger.kernel.org, David S. Miller
In-Reply-To: <AM5PR0501MB2449478FFD10730724CC32B9B1940@AM5PR0501MB2449.eurprd05.prod.outlook.com>
[-- Attachment #1: Type: text/plain, Size: 9000 bytes --]
From: Oleksandr Shamray
Sent: Tuesday, May 22, 2018 4:09 PM
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Arnd Bergmann <arnd@arndb.de>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>; devicetree <devicetree@vger.kernel.org>; openbmc@lists.ozlabs.org; Joel Stanley <joel@jms.id.au>; Jiří Pírko <jiri@resnulli.us>; Tobias Klauser <tklauser@distanz.ch>; open list:SERIAL DRIVERS <linux-serial@vger.kernel.org>; Vadim Pasternak <vadimp@mellanox.com>; system-sw-low-level <system-sw-low-level@mellanox.com>; Rob Herring <robh+dt@kernel.org>; openocd-devel-owner@lists.sourceforge.net; linux-api@vger.kernel.org; David S. Miller <davem@davemloft.net>; Mauro Carvalho Chehab <mchehab@kernel.org>; Jiri Pirko <jiri@mellanox.com>
Subject: RE: [patch v21 1/4] drivers: jtag: Add JTAG core driver
Hi Andy.
Thanks for review.
Please read my answers inline.
> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: 16 мая 2018 г. 0:22
> To: Oleksandr Shamray <oleksandrs@mellanox.com<mailto:oleksandrs@mellanox.com>>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org<mailto:gregkh@linuxfoundation.org>>; Arnd Bergmann
> <arnd@arndb.de<mailto:arnd@arndb.de>>; Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org<mailto:linux-kernel@vger.kernel.org>>; linux-arm Mailing List
> <linux-arm-kernel@lists.infradead.org<mailto:linux-arm-kernel@lists.infradead.org>>; devicetree
> <devicetree@vger.kernel.org<mailto:devicetree@vger.kernel.org>>; openbmc@lists.ozlabs.org<mailto:openbmc@lists.ozlabs.org>; Joel Stanley
> <joel@jms.id.au<mailto:joel@jms.id.au>>; Jiří Pírko <jiri@resnulli.us<mailto:jiri@resnulli.us>>; Tobias Klauser
> <tklauser@distanz.ch<mailto:tklauser@distanz.ch>>; open list:SERIAL DRIVERS <linux-
> serial@vger.kernel.org<mailto:serial@vger.kernel.org>>; Vadim Pasternak <vadimp@mellanox.com<mailto:vadimp@mellanox.com>>;
> system-sw-low-level <system-sw-low-level@mellanox.com<mailto:system-sw-low-level@mellanox.com>>; Rob Herring
> <robh+dt@kernel.org<mailto:robh+dt@kernel.org>>; openocd-devel-owner@lists.sourceforge.net<mailto:openocd-devel-owner@lists.sourceforge.net>;
> linux- api@vger.kernel.org<mailto:api@vger.kernel.org>; David S. Miller <davem@davemloft.net<mailto:davem@davemloft.net>>;
> Mauro Carvalho Chehab <mchehab@kernel.org<mailto:mchehab@kernel.org>>; Jiri Pirko
> <jiri@mellanox.com<mailto:jiri@mellanox.com>>
> Subject: Re: [patch v21 1/4] drivers: jtag: Add JTAG core driver
>
> On Tue, May 15, 2018 at 5:21 PM, Oleksandr Shamray
> <oleksandrs@mellanox.com<mailto:oleksandrs@mellanox.com>> wrote:
> > Initial patch for JTAG driver
> > JTAG class driver provide infrastructure to support
> > hardware/software JTAG platform drivers. It provide user layer API
> > interface for flashing and debugging external devices which equipped
> > with JTAG interface using standard transactions.
> >
> > Driver exposes set of IOCTL to user space for:
> > - XFER:
> > - SIR (Scan Instruction Register, IEEE 1149.1 Data Register scan);
> > - SDR (Scan Data Register, IEEE 1149.1 Instruction Register scan);
> > - RUNTEST (Forces the IEEE 1149.1 bus to a run state for a specified
> > number of clocks).
> > - SIOCFREQ/GIOCFREQ for setting and reading JTAG frequency.
> >
> > Driver core provides set of internal APIs for allocation and
> > registration:
> > - jtag_register;
> > - jtag_unregister;
> > - jtag_alloc;
> > - jtag_free;
> >
> > Platform driver on registration with jtag-core creates the next
> > entry in dev folder:
> > /dev/jtagX
>
> > 0xB0 all RATIO devices in development:
> > <mailto:vgo@ratio.de>
> > 0xB1 00-1F PPPoX <mailto:mostrows@styx.uwaterloo.ca>
> > +0xB2 00-0f linux/jtag.h JTAG driver
> > +
> > +<mailto:oleksandrs@mellanox.com>
>
> Consider to preserve style (upper vs. lower).
JTAG in code is lower (jtag) cane but in descriptions and notes it is upper (JTAG).
In all places which do not correspond to this I will fix.
>
> > + This provides basic core functionality support for JTAG class devices.
> > + Hardware that is equipped with a JTAG microcontroller can be
> > + supported by using this driver's interfaces.
> > + This driver exposes a set of IOCTLs to the user space for
> > + the following commands:
> > + SDR: (Scan Data Register) Performs an IEEE 1149.1 Data Register scan
> > + SIR: (Scan Instruction Register) Performs an IEEE 1149.1 Instruction
> > + Register scan.
> > + RUNTEST: Forces the IEEE 1149.1 bus to a run state for a specified
> > + number of clocks or a specified time period.
>
> Something feels wrong with formatting here.
>
Will fix
> > +#define MAX_JTAG_NAME_LEN (sizeof("jtag") + 5)
>
> Interesting definition. Why not to set to 10 explicitly? And why 10?
> (16 sounds better)
>
5 - is a max len of JTAG device id in device name. I will add define to it.
In general I don't see the case for the system with hundreds JTAG interfaces.
I will limit maximum jtag master to 255 devices and change id len to 3
#define MAX_JTAG_ID_STR_LEN 5
#define MAX_JTAG_NAME_LEN (sizeof("jtag") + MAX_JTAG_ID_STR_LEN)
> > +struct jtag {
> > + struct miscdevice miscdev;
>
> > + struct device *dev;
>
> Doesn't miscdev parent contain exactly this one?
Yes.
Will fix.
>
> > + const struct jtag_ops *ops;
> > + int id;
> > + bool opened;
> > + struct mutex open_lock;
> > + unsigned long priv[0];
> > +};
>
> > + err = copy_to_user(u64_to_user_ptr(xfer.tdio),
> > + (void *)(xfer_data), data_size);
>
> Redundant parens in one case. Check the rest similar places.
>
Yes.
> > +static int jtag_open(struct inode *inode, struct file *file) {
>
> > + struct jtag *jtag = container_of(file->private_data, struct jtag,
> > + miscdev);
>
> I would don't care about length and put it on one line.
>
But following to LINUX kernel style, it should be no longer than 80 symbols.
It will not pass by ./scripts/checkpatch.pl
Will it be OK to send a patch which failed 80 symbols limitation check?
> > + if (jtag->opened) {
> > + jtag->opened = true;
> > + jtag->opened = false;
>
> Can it be opened non exclusively several times? If so, this needs to
> be a ref counter instead.
It can be opened only once.
>
> > + if (!ops->idle || !ops->mode_set || !ops->status_get || !ops->xfer)
> > + return NULL;
>
> Are all of them mandatory?
>
Yes, except "mode_set"
Will remove mode_set from check
> > +int jtag_register(struct jtag *jtag)
>
> Perhaps devm_ variant.
Jtag driver uses miscdevice and related misc_register and misc_deregister calls for creation and destruction. There is no device object prior to call to misc_register, which could be used in devm_jtag_register.
Vadim: Keep size of line limited by 80 or 75 symbols
>
> > +#define jtag_u64_to_ptr(arg) ((void *)(uintptr_t)arg)
>
Redundant. Removed.
> Where is this used or supposed to be used?
>
> > +#define JTAG_MAX_XFER_DATA_LEN 65535
>
> Is this limitation from some spec?
> Otherwise why not to allow 64K?
>
It not limited by specification.
But we enforce an upper bound for the length here, to prevent users from draining kernel memory with giant buffers.
Vadim: your explanation is not relevant. You have there 64*1024 = 65535.
means 64K (maybe Andy considers 64K as 64*1000. But you can just
wrote him that this is 64*1024.
Vadim: Keep size of line limited by 80 or 75 symbols
> > +/**
> > + * struct jtag_ops - callbacks for jtag control functions:
> > + *
> > + * @freq_get: get frequency function. Filled by device driver
> > + * @freq_set: set frequency function. Filled by device driver
> > + * @status_get: set status function. Filled by device driver
> > + * @idle: set JTAG to idle state function. Filled by device driver
> > + * @xfer: send JTAG xfer function. Filled by device driver */
>
> Perhaps you need to describe which of them are _really_ mandatory and
> which are optional.
>
Ok
> --
> With Best Regards,
> Andy Shevchenko
Best Regards,
Oleksandr Shamray
[-- Attachment #2: Type: text/html, Size: 23298 bytes --]
^ permalink raw reply
* Re: [PATCH v4 06/12] media: dt-bindings: add bindings for i.MX7 media driver
From: Rui Miguel Silva @ 2018-05-22 13:19 UTC (permalink / raw)
To: Sakari Ailus
Cc: devel, devicetree, Greg Kroah-Hartman, Ryan Harkin, Rob Herring,
Rui Miguel Silva, Philipp Zabel, Steve Longerbeam, Fabio Estevam,
mchehab, Shawn Guo, linux-media
In-Reply-To: <20180518221346.fy4264hehvjjcd4y@kekkonen.localdomain>
Hi Sakari,
On Fri 18 May 2018 at 22:13, Sakari Ailus wrote:
> On Fri, May 18, 2018 at 09:27:58AM +0100, Rui Miguel Silva
> wrote:
>> > > +endpoint node
>> > > +-------------
>> > > +
>> > > +- data-lanes : (required) an array specifying active
>> > > physical
>> > > MIPI-CSI2
>> > > + data input lanes and their mapping to
>> > > logical lanes; the
>> > > + array's content is unused, only its
>> > > length is meaningful;
>
> Btw. do note that you may get a warning due to this from the
> CSI-2 bus
> property parsing code if the lane numbers are wrong.
>
>> > > +
>> > > +- fsl,csis-hs-settle : (optional) differential receiver
>> > > (HS-RX)
>> > > settle time;
>> >
>> > Could you calculate this, as other drivers do? It probably
>> > changes
>> > depending on the device runtime configuration.
>>
>> The only reference to possible values to this parameter is
>> given by
>> table in [0], can you point me out the formula for imx7 in the
>> documentation?
>
> I don't know imx7 but the other CSI-2 drivers need no such
> system specific
> configuration.
Hum, I think there is at least one more (which this is compliant)
that
also use this configuration parameter. [0]
---
Cheers,
Rui
[0]:
https://github.com/torvalds/linux/blob/a048a07d7f4535baa4cbad6bc024f175317ab938/Documentation/devicetree/bindings/media/samsung-mipi-csis.txt#L46
^ permalink raw reply
* [PATCH v3] arm64: allwinner: a64: Add Amarula A64-Relic initial support
From: Jagan Teki @ 2018-05-22 13:22 UTC (permalink / raw)
To: Maxime Ripard, Chen-Yu Tsai, Michael Trimarchi, Icenowy Zheng
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Jagan Teki
Amarula A64-Relic is Allwinner A64 based IoT device, which support
- Allwinner A64 Cortex-A53
- Mali-400MP2 GPU
- AXP803 PMIC
- 1GB DDR3 RAM
- 8GB eMMC
- AP6330 Wifi/BLE
- MIPI-DSI
- CSI: OV5640 sensor
- USB OTG
- 12V DC power supply
Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
---
Changes for v3:
- Use sun50i-a64-amarula-relic.dts name
- add eldo3 for dvdd-csi
- update dldo4 min voltage as 3.3v as per schematics
- use dldo3 name as dovdd-csi
- update aldo1, aldo2 voltages as per schematics
Changes for v2:
- Rename dts name to sun50i-a64-relic.dts which is simple to use
- Update dldo4 min voltage as 1.8
- Use licence year as 2018
arch/arm64/boot/dts/allwinner/Makefile | 1 +
.../dts/allwinner/sun50i-a64-amarula-relic.dts | 188 +++++++++++++++++++++
2 files changed, 189 insertions(+)
create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts
diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
index c31f90a49481..67ce8c500b2e 100644
--- a/arch/arm64/boot/dts/allwinner/Makefile
+++ b/arch/arm64/boot/dts/allwinner/Makefile
@@ -1,4 +1,5 @@
# SPDX-License-Identifier: GPL-2.0
+dtb-$(CONFIG_ARCH_SUNXI) += sun50i-a64-amarula-relic.dtb
dtb-$(CONFIG_ARCH_SUNXI) += sun50i-a64-bananapi-m64.dtb
dtb-$(CONFIG_ARCH_SUNXI) += sun50i-a64-nanopi-a64.dtb
dtb-$(CONFIG_ARCH_SUNXI) += sun50i-a64-olinuxino.dtb
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts
new file mode 100644
index 000000000000..6101ea83291c
--- /dev/null
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts
@@ -0,0 +1,188 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (C) 2018 Amarula Solutions B.V.
+ * Author: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
+ */
+
+/dts-v1/;
+
+#include "sun50i-a64.dtsi"
+
+#include <dt-bindings/gpio/gpio.h>
+
+/ {
+ model = "Amarula A64-Relic";
+ compatible = "amarula,a64-relic", "allwinner,sun50i-a64";
+
+ aliases {
+ serial0 = &uart0;
+ };
+
+ chosen {
+ stdout-path = "serial0:115200n8";
+ };
+};
+
+&ehci0 {
+ status = "okay";
+};
+
+&mmc2 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&mmc2_pins>;
+ vmmc-supply = <®_dcdc1>;
+ bus-width = <8>;
+ non-removable;
+ cap-mmc-hw-reset;
+ status = "okay";
+};
+
+&ohci0 {
+ status = "okay";
+};
+
+&r_rsb {
+ status = "okay";
+
+ axp803: pmic@3a3 {
+ compatible = "x-powers,axp803";
+ reg = <0x3a3>;
+ interrupt-parent = <&r_intc>;
+ interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
+ x-powers,drive-vbus-en; /* set N_VBUSEN as output pin */
+ };
+};
+
+#include "axp803.dtsi"
+
+®_aldo1 {
+ regulator-always-on;
+ regulator-min-microvolt = <2800000>;
+ regulator-max-microvolt = <2800000>;
+ regulator-name = "avdd-csi";
+};
+
+®_aldo2 {
+ regulator-always-on;
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-name = "vcc-pl";
+};
+
+®_aldo3 {
+ regulator-always-on;
+ regulator-min-microvolt = <3000000>;
+ regulator-max-microvolt = <3000000>;
+ regulator-name = "vcc-pll-avcc";
+};
+
+®_dcdc1 {
+ regulator-always-on;
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-name = "vcc-3v3";
+};
+
+®_dcdc2 {
+ regulator-always-on;
+ regulator-min-microvolt = <1040000>;
+ regulator-max-microvolt = <1300000>;
+ regulator-name = "vdd-cpux";
+};
+
+/* DCDC3 is polyphased with DCDC2 */
+
+®_dcdc5 {
+ regulator-always-on;
+ regulator-min-microvolt = <1500000>;
+ regulator-max-microvolt = <1500000>;
+ regulator-name = "vcc-dram";
+};
+
+®_dcdc6 {
+ regulator-always-on;
+ regulator-min-microvolt = <1100000>;
+ regulator-max-microvolt = <1100000>;
+ regulator-name = "vdd-sys";
+};
+
+®_dldo1 {
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-name = "vcc-hdmi-dsi-sensor";
+};
+
+®_dldo2 {
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-name = "vcc-mipi";
+};
+
+®_dldo3 {
+ regulator-min-microvolt = <2800000>;
+ regulator-max-microvolt = <2800000>;
+ regulator-name = "dovdd-csi";
+};
+
+®_dldo4 {
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-name = "vcc-wifi-io";
+};
+
+®_drivevbus {
+ regulator-name = "usb0-vbus";
+ status = "okay";
+};
+
+®_eldo1 {
+ regulator-always-on;
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-name = "cpvdd";
+};
+
+®_eldo3 {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-name = "dvdd-csi";
+};
+
+®_fldo1 {
+ regulator-min-microvolt = <1200000>;
+ regulator-max-microvolt = <1200000>;
+ regulator-name = "vcc-1v2-hsic";
+};
+
+/*
+ * The A64 chip cannot work without this regulator off, although
+ * it seems to be only driving the AR100 core.
+ * Maybe we don't still know well about CPUs domain.
+ */
+®_fldo2 {
+ regulator-always-on;
+ regulator-min-microvolt = <1100000>;
+ regulator-max-microvolt = <1100000>;
+ regulator-name = "vdd-cpus";
+};
+
+®_rtc_ldo {
+ regulator-name = "vcc-rtc";
+};
+
+&uart0 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart0_pins_a>;
+ status = "okay";
+};
+
+&usb_otg {
+ dr_mode = "peripheral";
+ status = "okay";
+};
+
+&usbphy {
+ usb0_id_det-gpios = <&pio 7 9 GPIO_ACTIVE_HIGH>; /* PH9 */
+ usb0_vbus-supply = <®_drivevbus>;
+ status = "okay";
+};
--
2.14.3
^ permalink raw reply related
* RE: RE: [patch v21 1/4] drivers: jtag: Add JTAG core driver
From: Vadim Pasternak @ 2018-05-22 13:22 UTC (permalink / raw)
To: Oleksandr Shamray, Andy Shevchenko
Cc: Greg Kroah-Hartman, Arnd Bergmann, Linux Kernel Mailing List,
linux-arm Mailing List, devicetree, openbmc@lists.ozlabs.org,
Joel Stanley, Jiří Pírko, Tobias Klauser,
open list:SERIAL DRIVERS, system-sw-low-level, Rob Herring,
openocd-devel-owner@lists.sourceforge.net,
linux-api@vger.kernel.org, David S. Miller
In-Reply-To: <HE1PR0502MB37535041B5E7C6FEE97FADC8A2940@HE1PR0502MB3753.eurprd05.prod.outlook.com>
[-- Attachment #1: Type: text/plain, Size: 10590 bytes --]
Sorry, for the spam, pleas, ignore it.
From: Vadim Pasternak
Sent: Tuesday, May 22, 2018 4:18 PM
To: Oleksandr Shamray <oleksandrs@mellanox.com>; Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Arnd Bergmann <arnd@arndb.de>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>; devicetree <devicetree@vger.kernel.org>; openbmc@lists.ozlabs.org; Joel Stanley <joel@jms.id.au>; Jiří Pírko <jiri@resnulli.us>; Tobias Klauser <tklauser@distanz.ch>; open list:SERIAL DRIVERS <linux-serial@vger.kernel.org>; system-sw-low-level <system-sw-low-level@mellanox.com>; Rob Herring <robh+dt@kernel.org>; openocd-devel-owner@lists.sourceforge.net; linux-api@vger.kernel.org; David S. Miller <davem@davemloft.net>; Mauro Carvalho Chehab <mchehab@kernel.org>; Jiri Pirko <jiri@mellanox.com>
Subject: RE: RE: [patch v21 1/4] drivers: jtag: Add JTAG core driver
From: Oleksandr Shamray
Sent: Tuesday, May 22, 2018 4:09 PM
To: Andy Shevchenko <andy.shevchenko@gmail.com<mailto:andy.shevchenko@gmail.com>>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org<mailto:gregkh@linuxfoundation.org>>; Arnd Bergmann <arnd@arndb.de<mailto:arnd@arndb.de>>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org<mailto:linux-kernel@vger.kernel.org>>; linux-arm Mailing List <linux-arm-kernel@lists.infradead.org<mailto:linux-arm-kernel@lists.infradead.org>>; devicetree <devicetree@vger.kernel.org<mailto:devicetree@vger.kernel.org>>; openbmc@lists.ozlabs.org<mailto:openbmc@lists.ozlabs.org>; Joel Stanley <joel@jms.id.au<mailto:joel@jms.id.au>>; Jiří Pírko <jiri@resnulli.us<mailto:jiri@resnulli.us>>; Tobias Klauser <tklauser@distanz.ch<mailto:tklauser@distanz.ch>>; open list:SERIAL DRIVERS <linux-serial@vger.kernel.org<mailto:linux-serial@vger.kernel.org>>; Vadim Pasternak <vadimp@mellanox.com<mailto:vadimp@mellanox.com>>; system-sw-low-level <system-sw-low-level@mellanox.com<mailto:system-sw-low-level@mellanox.com>>; Rob Herring <robh+dt@kernel.org<mailto:robh+dt@kernel.org>>; openocd-devel-owner@lists.sourceforge.net<mailto:openocd-devel-owner@lists.sourceforge.net>; linux-api@vger.kernel.org<mailto:linux-api@vger.kernel.org>; David S. Miller <davem@davemloft.net<mailto:davem@davemloft.net>>; Mauro Carvalho Chehab <mchehab@kernel.org<mailto:mchehab@kernel.org>>; Jiri Pirko <jiri@mellanox.com<mailto:jiri@mellanox.com>>
Subject: RE: [patch v21 1/4] drivers: jtag: Add JTAG core driver
Hi Andy.
Thanks for review.
Please read my answers inline.
> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: 16 мая 2018 г. 0:22
> To: Oleksandr Shamray <oleksandrs@mellanox.com<mailto:oleksandrs@mellanox.com>>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org<mailto:gregkh@linuxfoundation.org>>; Arnd Bergmann
> <arnd@arndb.de<mailto:arnd@arndb.de>>; Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org<mailto:linux-kernel@vger.kernel.org>>; linux-arm Mailing List
> <linux-arm-kernel@lists.infradead.org<mailto:linux-arm-kernel@lists.infradead.org>>; devicetree
> <devicetree@vger.kernel.org<mailto:devicetree@vger.kernel.org>>; openbmc@lists.ozlabs.org<mailto:openbmc@lists.ozlabs.org>; Joel Stanley
> <joel@jms.id.au<mailto:joel@jms.id.au>>; Jiří Pírko <jiri@resnulli.us<mailto:jiri@resnulli.us>>; Tobias Klauser
> <tklauser@distanz.ch<mailto:tklauser@distanz.ch>>; open list:SERIAL DRIVERS <linux-
> serial@vger.kernel.org<mailto:serial@vger.kernel.org>>; Vadim Pasternak <vadimp@mellanox.com<mailto:vadimp@mellanox.com>>;
> system-sw-low-level <system-sw-low-level@mellanox.com<mailto:system-sw-low-level@mellanox.com>>; Rob Herring
> <robh+dt@kernel.org<mailto:robh+dt@kernel.org>>; openocd-devel-owner@lists.sourceforge.net<mailto:openocd-devel-owner@lists.sourceforge.net>;
> linux- api@vger.kernel.org<mailto:api@vger.kernel.org>; David S. Miller <davem@davemloft.net<mailto:davem@davemloft.net>>;
> Mauro Carvalho Chehab <mchehab@kernel.org<mailto:mchehab@kernel.org>>; Jiri Pirko
> <jiri@mellanox.com<mailto:jiri@mellanox.com>>
> Subject: Re: [patch v21 1/4] drivers: jtag: Add JTAG core driver
>
> On Tue, May 15, 2018 at 5:21 PM, Oleksandr Shamray
> <oleksandrs@mellanox.com<mailto:oleksandrs@mellanox.com>> wrote:
> > Initial patch for JTAG driver
> > JTAG class driver provide infrastructure to support
> > hardware/software JTAG platform drivers. It provide user layer API
> > interface for flashing and debugging external devices which equipped
> > with JTAG interface using standard transactions.
> >
> > Driver exposes set of IOCTL to user space for:
> > - XFER:
> > - SIR (Scan Instruction Register, IEEE 1149.1 Data Register scan);
> > - SDR (Scan Data Register, IEEE 1149.1 Instruction Register scan);
> > - RUNTEST (Forces the IEEE 1149.1 bus to a run state for a specified
> > number of clocks).
> > - SIOCFREQ/GIOCFREQ for setting and reading JTAG frequency.
> >
> > Driver core provides set of internal APIs for allocation and
> > registration:
> > - jtag_register;
> > - jtag_unregister;
> > - jtag_alloc;
> > - jtag_free;
> >
> > Platform driver on registration with jtag-core creates the next
> > entry in dev folder:
> > /dev/jtagX
>
> > 0xB0 all RATIO devices in development:
> > <mailto:vgo@ratio.de>
> > 0xB1 00-1F PPPoX <mailto:mostrows@styx.uwaterloo.ca>
> > +0xB2 00-0f linux/jtag.h JTAG driver
> > +
> > +<mailto:oleksandrs@mellanox.com>
>
> Consider to preserve style (upper vs. lower).
JTAG in code is lower (jtag) cane but in descriptions and notes it is upper (JTAG).
In all places which do not correspond to this I will fix.
>
> > + This provides basic core functionality support for JTAG class devices.
> > + Hardware that is equipped with a JTAG microcontroller can be
> > + supported by using this driver's interfaces.
> > + This driver exposes a set of IOCTLs to the user space for
> > + the following commands:
> > + SDR: (Scan Data Register) Performs an IEEE 1149.1 Data Register scan
> > + SIR: (Scan Instruction Register) Performs an IEEE 1149.1 Instruction
> > + Register scan.
> > + RUNTEST: Forces the IEEE 1149.1 bus to a run state for a specified
> > + number of clocks or a specified time period.
>
> Something feels wrong with formatting here.
>
Will fix
> > +#define MAX_JTAG_NAME_LEN (sizeof("jtag") + 5)
>
> Interesting definition. Why not to set to 10 explicitly? And why 10?
> (16 sounds better)
>
5 - is a max len of JTAG device id in device name. I will add define to it.
In general I don't see the case for the system with hundreds JTAG interfaces.
I will limit maximum jtag master to 255 devices and change id len to 3
#define MAX_JTAG_ID_STR_LEN 5
#define MAX_JTAG_NAME_LEN (sizeof("jtag") + MAX_JTAG_ID_STR_LEN)
> > +struct jtag {
> > + struct miscdevice miscdev;
>
> > + struct device *dev;
>
> Doesn't miscdev parent contain exactly this one?
Yes.
Will fix.
>
> > + const struct jtag_ops *ops;
> > + int id;
> > + bool opened;
> > + struct mutex open_lock;
> > + unsigned long priv[0];
> > +};
>
> > + err = copy_to_user(u64_to_user_ptr(xfer.tdio),
> > + (void *)(xfer_data), data_size);
>
> Redundant parens in one case. Check the rest similar places.
>
Yes.
> > +static int jtag_open(struct inode *inode, struct file *file) {
>
> > + struct jtag *jtag = container_of(file->private_data, struct jtag,
> > + miscdev);
>
> I would don't care about length and put it on one line.
>
But following to LINUX kernel style, it should be no longer than 80 symbols.
It will not pass by ./scripts/checkpatch.pl
Will it be OK to send a patch which failed 80 symbols limitation check?
> > + if (jtag->opened) {
> > + jtag->opened = true;
> > + jtag->opened = false;
>
> Can it be opened non exclusively several times? If so, this needs to
> be a ref counter instead.
It can be opened only once.
>
> > + if (!ops->idle || !ops->mode_set || !ops->status_get || !ops->xfer)
> > + return NULL;
>
> Are all of them mandatory?
>
Yes, except "mode_set"
Will remove mode_set from check
> > +int jtag_register(struct jtag *jtag)
>
> Perhaps devm_ variant.
Jtag driver uses miscdevice and related misc_register and misc_deregister calls for creation and destruction. There is no device object prior to call to misc_register, which could be used in devm_jtag_register.
Vadim: Keep size of line limited by 80 or 75 symbols
>
> > +#define jtag_u64_to_ptr(arg) ((void *)(uintptr_t)arg)
>
Redundant. Removed.
> Where is this used or supposed to be used?
>
> > +#define JTAG_MAX_XFER_DATA_LEN 65535
>
> Is this limitation from some spec?
> Otherwise why not to allow 64K?
>
It not limited by specification.
But we enforce an upper bound for the length here, to prevent users from draining kernel memory with giant buffers.
Vadim: your explanation is not relevant. You have there 64*1024 = 65535.
means 64K (maybe Andy considers 64K as 64*1000. But you can just
wrote him that this is 64*1024.
Vadim: Keep size of line limited by 80 or 75 symbols
> > +/**
> > + * struct jtag_ops - callbacks for jtag control functions:
> > + *
> > + * @freq_get: get frequency function. Filled by device driver
> > + * @freq_set: set frequency function. Filled by device driver
> > + * @status_get: set status function. Filled by device driver
> > + * @idle: set JTAG to idle state function. Filled by device driver
> > + * @xfer: send JTAG xfer function. Filled by device driver */
>
> Perhaps you need to describe which of them are _really_ mandatory and
> which are optional.
>
Ok
> --
> With Best Regards,
> Andy Shevchenko
Best Regards,
Oleksandr Shamray
[-- Attachment #2: Type: text/html, Size: 25826 bytes --]
^ permalink raw reply
* Re: [PATCH v4 2/3] arm64: dts: renesas: draak: Describe CVBS input
From: Simon Horman @ 2018-05-22 13:24 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Jacopo Mondi, niklas.soderlund, geert, magnus.damm, robh+dt,
linux-renesas-soc, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <1958464.yiJe0NDHa3@avalon>
On Mon, May 21, 2018 at 05:50:50PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Monday, 21 May 2018 17:45:41 EEST Jacopo Mondi wrote:
> > Describe CVBS video input through analog video decoder ADV7180
> > connected to video input interface VIN4.
> >
> > The video input signal path is shared with HDMI video input, and
> > selected by on-board switches SW-53 and SW-54 with CVBS input selected
> > by the default switches configuration.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Thanks Niklas, applied.
^ permalink raw reply
* Re: [PATCH v4 3/3] arm64: dts: renesas: draak: Describe HDMI input
From: Simon Horman @ 2018-05-22 13:24 UTC (permalink / raw)
To: Jacopo Mondi
Cc: niklas.soderlund, laurent.pinchart, geert, magnus.damm, robh+dt,
linux-renesas-soc, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <1526913942-15426-4-git-send-email-jacopo+renesas@jmondi.org>
On Mon, May 21, 2018 at 04:45:42PM +0200, Jacopo Mondi wrote:
> Describe HDMI input connector and ADV7612 HDMI decoder installed on
> R-Car Gen3 Draak board.
>
> The video signal routing to the HDMI decoder to the video input interface
> VIN4 is multiplexed with CVBS input path, and enabled/disabled through
> on-board switches SW-49, SW-50, SW-51 and SW-52.
>
> As the default board switches configuration connects CVBS input to VIN4,
> leave the HDMI decoder unconnected in DTS.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Thanks Jacopo,
applied.
^ permalink raw reply
* Re: [PATCH v4 2/3] arm64: dts: renesas: draak: Describe CVBS input
From: Simon Horman @ 2018-05-22 13:25 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Jacopo Mondi, niklas.soderlund, geert, magnus.damm, robh+dt,
linux-renesas-soc, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20180522132411.7szrsk4pxvwrqrxf@verge.net.au>
On Tue, May 22, 2018 at 03:24:13PM +0200, Simon Horman wrote:
> On Mon, May 21, 2018 at 05:50:50PM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Monday, 21 May 2018 17:45:41 EEST Jacopo Mondi wrote:
> > > Describe CVBS video input through analog video decoder ADV7180
> > > connected to video input interface VIN4.
> > >
> > > The video input signal path is shared with HDMI video input, and
> > > selected by on-board switches SW-53 and SW-54 with CVBS input selected
> > > by the default switches configuration.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Thanks Niklas, applied.
Sorry, I meant thanks Jacopo, Niklas and Laurent!
^ permalink raw reply
* [RESEND PATCH 0/4] PCI: cadence: Host and EP driver updates for PHY and power management
From: Alan Douglas @ 2018-05-22 13:26 UTC (permalink / raw)
To: bhelgaas, kishon, lorenzo.pieralisi, linux-pci
Cc: devicetree, linux-kernel, cyrille.pitchen, Alan Douglas
This is a series of patches for the cadence PCIe host and EP drivers, to:
- Add optional list of generic PHYs to host and EP drivers
- Add PHY bindings to devicetree
- Add Power Management ops, which will enable/disable PHYs if present
- Update cdns_pcie_writel function signature
Changes in v2:
Split commit into four patches
Re-based on v4.17-rc1
Alan Douglas (4):
PCI: cadence: Update cdns_pcie_writel function signature
PCI: cadence: Add generic PHY support to host and EP drivers
dt-bindings: PCI: cadence: Add DT bindings for optional PHYs
PCI: cadence: Add Power Management ops for host and EP
.../devicetree/bindings/pci/cdns,cdns-pcie-ep.txt | 4 +
.../bindings/pci/cdns,cdns-pcie-host.txt | 2 +
drivers/pci/cadence/pcie-cadence-ep.c | 15 ++-
drivers/pci/cadence/pcie-cadence-host.c | 34 ++++++
drivers/pci/cadence/pcie-cadence.c | 123 +++++++++++++++++++++
drivers/pci/cadence/pcie-cadence.h | 13 ++-
6 files changed, 189 insertions(+), 2 deletions(-)
--
2.2.2
^ permalink raw reply
* [RESEND PATCH 1/4] PCI: cadence: Update cdns_pcie_writel function signature
From: Alan Douglas @ 2018-05-22 13:26 UTC (permalink / raw)
To: bhelgaas, kishon, lorenzo.pieralisi, linux-pci
Cc: devicetree, linux-kernel, cyrille.pitchen, Alan Douglas
Change cdns_pcie_writel() signature, u16 value changed to u32, since
this function should write a long value
Signed-off-by: Alan Douglas <adouglas@cadence.com>
---
drivers/pci/cadence/pcie-cadence.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/cadence/pcie-cadence.h b/drivers/pci/cadence/pcie-cadence.h
index 4bb2733..ed336cc 100644
--- a/drivers/pci/cadence/pcie-cadence.h
+++ b/drivers/pci/cadence/pcie-cadence.h
@@ -279,7 +279,7 @@ static inline void cdns_pcie_ep_fn_writew(struct cdns_pcie *pcie, u8 fn,
}
static inline void cdns_pcie_ep_fn_writel(struct cdns_pcie *pcie, u8 fn,
- u32 reg, u16 value)
+ u32 reg, u32 value)
{
writel(value, pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg);
}
--
2.2.2
^ permalink raw reply related
* [RESEND PATCH 2/4] PCI: cadence: Add generic PHY support to host and EP drivers
From: Alan Douglas @ 2018-05-22 13:27 UTC (permalink / raw)
To: bhelgaas, kishon, lorenzo.pieralisi, linux-pci
Cc: devicetree, linux-kernel, cyrille.pitchen, Alan Douglas
If PHYs are present, they will be initialized and enabled in driver probe,
and disabled in driver shutdown.
Signed-off-by: Alan Douglas <adouglas@cadence.com>
---
drivers/pci/cadence/pcie-cadence-ep.c | 14 ++++-
drivers/pci/cadence/pcie-cadence-host.c | 31 +++++++++++
drivers/pci/cadence/pcie-cadence.c | 93 +++++++++++++++++++++++++++++++++
drivers/pci/cadence/pcie-cadence.h | 7 +++
4 files changed, 144 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/cadence/pcie-cadence-ep.c b/drivers/pci/cadence/pcie-cadence-ep.c
index 3d8283e..2581caf 100644
--- a/drivers/pci/cadence/pcie-cadence-ep.c
+++ b/drivers/pci/cadence/pcie-cadence-ep.c
@@ -439,6 +439,7 @@ static int cdns_pcie_ep_probe(struct platform_device *pdev)
struct pci_epc *epc;
struct resource *res;
int ret;
+ int phy_count;
ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
if (!ep)
@@ -472,6 +473,12 @@ static int cdns_pcie_ep_probe(struct platform_device *pdev)
if (!ep->ob_addr)
return -ENOMEM;
+ ret = cdns_pcie_init_phy(dev, pcie);
+ if (ret) {
+ dev_err(dev, "failed to init phy\n");
+ return ret;
+ }
+ platform_set_drvdata(pdev, pcie);
pm_runtime_enable(dev);
ret = pm_runtime_get_sync(dev);
if (ret < 0) {
@@ -520,6 +527,10 @@ static int cdns_pcie_ep_probe(struct platform_device *pdev)
err_get_sync:
pm_runtime_disable(dev);
+ cdns_pcie_disable_phy(pcie);
+ phy_count = pcie->phy_count;
+ while (phy_count--)
+ device_link_del(pcie->link[phy_count]);
return ret;
}
@@ -527,6 +538,7 @@ static int cdns_pcie_ep_probe(struct platform_device *pdev)
static void cdns_pcie_ep_shutdown(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
+ struct cdns_pcie *pcie = dev_get_drvdata(dev);
int ret;
ret = pm_runtime_put_sync(dev);
@@ -535,7 +547,7 @@ static void cdns_pcie_ep_shutdown(struct platform_device *pdev)
pm_runtime_disable(dev);
- /* The PCIe controller can't be disabled. */
+ cdns_pcie_disable_phy(pcie);
}
static struct platform_driver cdns_pcie_ep_driver = {
diff --git a/drivers/pci/cadence/pcie-cadence-host.c b/drivers/pci/cadence/pcie-cadence-host.c
index a4ebbd3..7536926a 100644
--- a/drivers/pci/cadence/pcie-cadence-host.c
+++ b/drivers/pci/cadence/pcie-cadence-host.c
@@ -58,6 +58,9 @@ static void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
return pcie->reg_base + (where & 0xfff);
}
+ /* Check that the link is up */
+ if (!(cdns_pcie_readl(pcie, CDNS_PCIE_LM_BASE) & 0x1))
+ return NULL;
/* Update Output registers for AXI region 0. */
addr0 = CDNS_PCIE_AT_OB_REGION_PCI_ADDR0_NBITS(12) |
@@ -239,6 +242,7 @@ static int cdns_pcie_host_probe(struct platform_device *pdev)
struct cdns_pcie *pcie;
struct resource *res;
int ret;
+ int phy_count;
bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc));
if (!bridge)
@@ -290,6 +294,13 @@ static int cdns_pcie_host_probe(struct platform_device *pdev)
}
pcie->mem_res = res;
+ ret = cdns_pcie_init_phy(dev, pcie);
+ if (ret) {
+ dev_err(dev, "failed to init phy\n");
+ return ret;
+ }
+ platform_set_drvdata(pdev, pcie);
+
pm_runtime_enable(dev);
ret = pm_runtime_get_sync(dev);
if (ret < 0) {
@@ -322,15 +333,35 @@ static int cdns_pcie_host_probe(struct platform_device *pdev)
err_get_sync:
pm_runtime_disable(dev);
+ cdns_pcie_disable_phy(pcie);
+ phy_count = pcie->phy_count;
+ while (phy_count--)
+ device_link_del(pcie->link[phy_count]);
return ret;
}
+static void cdns_pcie_shutdown(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct cdns_pcie *pcie = dev_get_drvdata(dev);
+ int ret;
+
+ ret = pm_runtime_put_sync(dev);
+ if (ret < 0)
+ dev_dbg(dev, "pm_runtime_put_sync failed\n");
+
+ pm_runtime_disable(dev);
+ cdns_pcie_disable_phy(pcie);
+}
+
+
static struct platform_driver cdns_pcie_host_driver = {
.driver = {
.name = "cdns-pcie-host",
.of_match_table = cdns_pcie_host_of_match,
},
.probe = cdns_pcie_host_probe,
+ .shutdown = cdns_pcie_shutdown,
};
builtin_platform_driver(cdns_pcie_host_driver);
diff --git a/drivers/pci/cadence/pcie-cadence.c b/drivers/pci/cadence/pcie-cadence.c
index 138d113..681609a 100644
--- a/drivers/pci/cadence/pcie-cadence.c
+++ b/drivers/pci/cadence/pcie-cadence.c
@@ -124,3 +124,96 @@ void cdns_pcie_reset_outbound_region(struct cdns_pcie *pcie, u32 r)
cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_CPU_ADDR0(r), 0);
cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_CPU_ADDR1(r), 0);
}
+
+void cdns_pcie_disable_phy(struct cdns_pcie *pcie)
+{
+ int i = pcie->phy_count;
+
+ while (i--) {
+ phy_power_off(pcie->phy[i]);
+ phy_exit(pcie->phy[i]);
+ }
+}
+
+int cdns_pcie_enable_phy(struct cdns_pcie *pcie)
+{
+ int ret;
+ int i;
+
+ for (i = 0; i < pcie->phy_count; i++) {
+ ret = phy_init(pcie->phy[i]);
+ if (ret < 0)
+ goto err_phy;
+
+ ret = phy_power_on(pcie->phy[i]);
+ if (ret < 0) {
+ phy_exit(pcie->phy[i]);
+ goto err_phy;
+ }
+ }
+
+ return 0;
+
+err_phy:
+ while (--i >= 0) {
+ phy_power_off(pcie->phy[i]);
+ phy_exit(pcie->phy[i]);
+ }
+
+ return ret;
+}
+
+int cdns_pcie_init_phy(struct device *dev, struct cdns_pcie *pcie)
+{
+ struct device_node *np = dev->of_node;
+ int phy_count;
+ struct phy **phy;
+ struct device_link **link;
+ int i;
+ int ret;
+ const char *name;
+
+ phy_count = of_property_count_strings(np, "phy-names");
+ if (phy_count < 1) {
+ dev_err(dev, "no phy-names. PHY will not be initialized\n");
+ pcie->phy_count = 0;
+ return 0;
+ }
+
+ phy = devm_kzalloc(dev, sizeof(*phy) * phy_count, GFP_KERNEL);
+ if (!phy)
+ return -ENOMEM;
+
+ link = devm_kzalloc(dev, sizeof(*link) * phy_count, GFP_KERNEL);
+ if (!link)
+ return -ENOMEM;
+
+ for (i = 0; i < phy_count; i++) {
+ of_property_read_string_index(np, "phy-names", i, &name);
+ phy[i] = devm_phy_get(dev, name);
+ if (IS_ERR(phy))
+ return PTR_ERR(phy);
+
+ link[i] = device_link_add(dev, &phy[i]->dev, DL_FLAG_STATELESS);
+ if (!link[i]) {
+ ret = -EINVAL;
+ goto err_link;
+ }
+ }
+
+ pcie->phy_count = phy_count;
+ pcie->phy = phy;
+ pcie->link = link;
+
+ ret = cdns_pcie_enable_phy(pcie);
+ if (ret)
+ goto err_link;
+
+ return 0;
+
+err_link:
+ while (--i >= 0)
+ device_link_del(link[i]);
+
+ return ret;
+}
diff --git a/drivers/pci/cadence/pcie-cadence.h b/drivers/pci/cadence/pcie-cadence.h
index ed336cc..b342c80 100644
--- a/drivers/pci/cadence/pcie-cadence.h
+++ b/drivers/pci/cadence/pcie-cadence.h
@@ -8,6 +8,7 @@
#include <linux/kernel.h>
#include <linux/pci.h>
+#include <linux/phy/phy.h>
/*
* Local Management Registers
@@ -229,6 +230,9 @@ struct cdns_pcie {
struct resource *mem_res;
bool is_rc;
u8 bus;
+ int phy_count;
+ struct phy **phy;
+ struct device_link **link;
};
/* Register access */
@@ -307,5 +311,8 @@ void cdns_pcie_set_outbound_region_for_normal_msg(struct cdns_pcie *pcie, u8 fn,
u32 r, u64 cpu_addr);
void cdns_pcie_reset_outbound_region(struct cdns_pcie *pcie, u32 r);
+void cdns_pcie_disable_phy(struct cdns_pcie *pcie);
+int cdns_pcie_enable_phy(struct cdns_pcie *pcie);
+int cdns_pcie_init_phy(struct device *dev, struct cdns_pcie *pcie);
#endif /* _PCIE_CADENCE_H */
--
2.2.2
^ permalink raw reply related
* [PATCH/RFC] ARM: dts: r8a7791: Move enable-method to CPU nodes
From: Geert Uytterhoeven @ 2018-05-22 13:29 UTC (permalink / raw)
To: Simon Horman, Magnus Damm, Rob Herring, Mark Rutland,
Lorenzo Pieralisi, Stephen Boyd
Cc: linux-renesas-soc, linux-arm-kernel, devicetree, linux-kernel,
Geert Uytterhoeven
According to Documentation/devicetree/bindings/arm/cpus.txt, the
"enable-method" property should be a property of the individual CPU
nodes, not of the parent "cpus" node. However, on R-Car M2-W (and on
several other arm32 SoCs), the property is tied to the "cpus" node
instead.
Secondary CPU bringup and CPU hot (un)plug work regardless, as
arm_dt_init_cpu_maps() falls back to looking in the "cpus" node.
The cpuidle code does not have such a fallback, so it does not detect
the enable-method. Note that cpuidle does not support the
"renesas,apmu" enable-method yet, so for now this does not make any
difference.
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Arm64 and powerpc do not have such a fallback, but SH has, like arm32.
This is marked RFC, as the alternative is to update the DT bindings to
keep the status quo.
---
arch/arm/boot/dts/r8a7791.dtsi | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/r8a7791.dtsi b/arch/arm/boot/dts/r8a7791.dtsi
index d568bd22d6cbd855..b214cb8f52e47109 100644
--- a/arch/arm/boot/dts/r8a7791.dtsi
+++ b/arch/arm/boot/dts/r8a7791.dtsi
@@ -71,7 +71,6 @@
cpus {
#address-cells = <1>;
#size-cells = <0>;
- enable-method = "renesas,apmu";
cpu0: cpu@0 {
device_type = "cpu";
@@ -83,6 +82,7 @@
clock-latency = <300000>; /* 300 us */
power-domains = <&sysc R8A7791_PD_CA15_CPU0>;
next-level-cache = <&L2_CA15>;
+ enable-method = "renesas,apmu";
/* kHz - uV - OPPs unknown yet */
operating-points = <1500000 1000000>,
@@ -101,6 +101,7 @@
clocks = <&cpg CPG_CORE R8A7791_CLK_Z>;
power-domains = <&sysc R8A7791_PD_CA15_CPU1>;
next-level-cache = <&L2_CA15>;
+ enable-method = "renesas,apmu";
};
L2_CA15: cache-controller-0 {
--
2.7.4
^ permalink raw reply related
* [RESEND PATCH 3/4] dt-bindings: PCI: cadence: Add DT bindings for optional PHYs
From: Alan Douglas @ 2018-05-22 13:32 UTC (permalink / raw)
To: bhelgaas, kishon, lorenzo.pieralisi, linux-pci, devicetree
Cc: linux-kernel, cyrille.pitchen, Alan Douglas
Update DT documentation to include optional PHYs for cadence PCIe
host and endpoint controllers.
Signed-off-by: Alan Douglas <adouglas@cadence.com>
---
Documentation/devicetree/bindings/pci/cdns,cdns-pcie-ep.txt | 4 ++++
Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt | 2 ++
2 files changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-ep.txt b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-ep.txt
index 9a305237..e40c635 100644
--- a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-ep.txt
+++ b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-ep.txt
@@ -9,6 +9,8 @@ Required properties:
Optional properties:
- max-functions: Maximum number of functions that can be configured (default 1).
+- phys: From PHY bindings: List of Generic PHY phandles.
+- phy-names: List of names to identify the PHY.
Example:
@@ -19,4 +21,6 @@ pcie@fc000000 {
reg-names = "reg", "mem";
cdns,max-outbound-regions = <16>;
max-functions = /bits/ 8 <8>;
+ phys = <&ep_phy0 &ep_phy1>;
+ phy-names = "pcie-lane0","pcie-lane1";
};
diff --git a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
index 20a33f3..c0ca4c1 100644
--- a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
+++ b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
@@ -24,6 +24,8 @@ Optional properties:
translations (default 32)
- vendor-id: The PCI vendor ID (16 bits, default is design dependent)
- device-id: The PCI device ID (16 bits, default is design dependent)
+- phys: From PHY bindings: List of Generic PHY phandles.
+- phy-names: List of names to identify the PHY.
Example:
--
2.2.2
^ permalink raw reply related
* [RESEND PATCH 4/4] PCI: cadence: Add Power Management ops for host and EP
From: Alan Douglas @ 2018-05-22 13:33 UTC (permalink / raw)
To: bhelgaas, kishon, lorenzo.pieralisi, linux-pci
Cc: devicetree, linux-kernel, cyrille.pitchen, Alan Douglas
These PM ops will enable/disable the optional PHYs if present. The
AXI link-down register in the host driver is now cleared in
cdns_pci_map_bus since the link-down bit will be set if the PHY has
been disabled. It is not cleared when enabling the PHY, since the
link will not yet be up.
Signed-off-by: Alan Douglas <adouglas@cadence.com>
---
drivers/pci/cadence/pcie-cadence-ep.c | 1 +
drivers/pci/cadence/pcie-cadence-host.c | 3 +++
drivers/pci/cadence/pcie-cadence.c | 30 ++++++++++++++++++++++++++++++
drivers/pci/cadence/pcie-cadence.h | 4 ++++
4 files changed, 38 insertions(+)
diff --git a/drivers/pci/cadence/pcie-cadence-ep.c b/drivers/pci/cadence/pcie-cadence-ep.c
index 2581caf..e74b8a4 100644
--- a/drivers/pci/cadence/pcie-cadence-ep.c
+++ b/drivers/pci/cadence/pcie-cadence-ep.c
@@ -554,6 +554,7 @@ static struct platform_driver cdns_pcie_ep_driver = {
.driver = {
.name = "cdns-pcie-ep",
.of_match_table = cdns_pcie_ep_of_match,
+ .pm = &cdns_pcie_pm_ops,
},
.probe = cdns_pcie_ep_probe,
.shutdown = cdns_pcie_ep_shutdown,
diff --git a/drivers/pci/cadence/pcie-cadence-host.c b/drivers/pci/cadence/pcie-cadence-host.c
index 7536926a..a8b5eda 100644
--- a/drivers/pci/cadence/pcie-cadence-host.c
+++ b/drivers/pci/cadence/pcie-cadence-host.c
@@ -61,6 +61,8 @@ static void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
/* Check that the link is up */
if (!(cdns_pcie_readl(pcie, CDNS_PCIE_LM_BASE) & 0x1))
return NULL;
+ /* Clear AXI link-down status */
+ cdns_pcie_writel(pcie, CDNS_PCIE_AT_LINKDOWN, 0x0);
/* Update Output registers for AXI region 0. */
addr0 = CDNS_PCIE_AT_OB_REGION_PCI_ADDR0_NBITS(12) |
@@ -360,6 +362,7 @@ static struct platform_driver cdns_pcie_host_driver = {
.driver = {
.name = "cdns-pcie-host",
.of_match_table = cdns_pcie_host_of_match,
+ .pm = &cdns_pcie_pm_ops,
},
.probe = cdns_pcie_host_probe,
.shutdown = cdns_pcie_shutdown,
diff --git a/drivers/pci/cadence/pcie-cadence.c b/drivers/pci/cadence/pcie-cadence.c
index 681609a..7a34780 100644
--- a/drivers/pci/cadence/pcie-cadence.c
+++ b/drivers/pci/cadence/pcie-cadence.c
@@ -217,3 +217,33 @@ int cdns_pcie_init_phy(struct device *dev, struct cdns_pcie *pcie)
return ret;
}
+
+#ifdef CONFIG_PM_SLEEP
+static int cdns_pcie_suspend_noirq(struct device *dev)
+{
+ struct cdns_pcie *pcie = dev_get_drvdata(dev);
+
+ cdns_pcie_disable_phy(pcie);
+
+ return 0;
+}
+
+static int cdns_pcie_resume_noirq(struct device *dev)
+{
+ struct cdns_pcie *pcie = dev_get_drvdata(dev);
+ int ret;
+
+ ret = cdns_pcie_enable_phy(pcie);
+ if (ret) {
+ dev_err(dev, "failed to enable phy\n");
+ return ret;
+ }
+
+ return 0;
+}
+#endif
+
+const struct dev_pm_ops cdns_pcie_pm_ops = {
+ SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(cdns_pcie_suspend_noirq,
+ cdns_pcie_resume_noirq)
+};
diff --git a/drivers/pci/cadence/pcie-cadence.h b/drivers/pci/cadence/pcie-cadence.h
index b342c80..ae6bf2a 100644
--- a/drivers/pci/cadence/pcie-cadence.h
+++ b/drivers/pci/cadence/pcie-cadence.h
@@ -166,6 +166,9 @@
#define CDNS_PCIE_AT_IB_RP_BAR_ADDR1(bar) \
(CDNS_PCIE_AT_BASE + 0x0804 + (bar) * 0x0008)
+/* AXI link down register */
+#define CDNS_PCIE_AT_LINKDOWN (CDNS_PCIE_AT_BASE + 0x0824)
+
enum cdns_pcie_rp_bar {
RP_BAR0,
RP_BAR1,
@@ -314,5 +317,6 @@ void cdns_pcie_reset_outbound_region(struct cdns_pcie *pcie, u32 r);
void cdns_pcie_disable_phy(struct cdns_pcie *pcie);
int cdns_pcie_enable_phy(struct cdns_pcie *pcie);
int cdns_pcie_init_phy(struct device *dev, struct cdns_pcie *pcie);
+extern const struct dev_pm_ops cdns_pcie_pm_ops;
#endif /* _PCIE_CADENCE_H */
--
2.2.2
^ permalink raw reply related
* Re: [RESEND PATCH 2/5] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver
From: Dmitry Osipenko @ 2018-05-22 13:34 UTC (permalink / raw)
To: Stefan Agner
Cc: boris.brezillon, dwmw2, computersforpeace, marek.vasut, robh+dt,
mark.rutland, thierry.reding, mturquette, sboyd, dev,
miquel.raynal, richard, marcel, krzk, benjamin.lindqvist,
jonathanh, pdeschrijver, pgaikwad, mirza.krak, linux-mtd,
linux-tegra, devicetree, linux-kernel, linux-clk
In-Reply-To: <2e858be88e00e26495fe47c6cd5cd70c@agner.ch>
On 22.05.2018 15:19, Stefan Agner wrote:
> [review sent to my first patch sent off-ml, moving to ml thread]
>
> On 21.05.2018 16:05, Dmitry Osipenko wrote:
>> Hello Stefan,
>>
>> I don't have expertise to review the actual NAND-related driver logic, so I only
>> reviewed the basics. The driver code looks good to me, though I've couple minor
>> comments.
>>
>> On 21.05.2018 03:16, Stefan Agner wrote:
>>> Add support for the NAND flash controller found on NVIDIA
>>> Tegra 2 SoCs. This implementation does not make use of the
>>> command queue feature. Regular operations/data transfers are
>>> done in PIO mode. Page read/writes with hardware ECC make
>>> use of the DMA for data transfer.
>>>
>>> Signed-off-by: Lucas Stach <dev@lynxeye.de>
>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>> ---
>>> MAINTAINERS | 7 +
>>> drivers/mtd/nand/raw/Kconfig | 6 +
>>> drivers/mtd/nand/raw/Makefile | 1 +
>>> drivers/mtd/nand/raw/tegra_nand.c | 915 ++++++++++++++++++++++++++++++
>>> 4 files changed, 929 insertions(+)
>>> create mode 100644 drivers/mtd/nand/raw/tegra_nand.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 58b9861ccf99..a65739681279 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -13844,6 +13844,13 @@ M: Laxman Dewangan <ldewangan@nvidia.com>
>>> S: Supported
>>> F: drivers/input/keyboard/tegra-kbc.c
>>>
>>> +TEGRA NAND DRIVER
>>> +M: Stefan Agner <stefan@agner.ch>
>>> +M: Lucas Stach <dev@lynxeye.de>
>>> +S: Maintained
>>> +F: Documentation/devicetree/bindings/mtd/nvidia,tegra20-nand.txt
>>> +F: drivers/mtd/nand/tegra_nand.c
>>> +
>>> TEGRA PWM DRIVER
>>> M: Thierry Reding <thierry.reding@gmail.com>
>>> S: Supported
>>> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
>>> index 19a2b283fbbe..bd56264233ca 100644
>>> --- a/drivers/mtd/nand/raw/Kconfig
>>> +++ b/drivers/mtd/nand/raw/Kconfig
>>> @@ -534,4 +534,10 @@ config MTD_NAND_MTK
>>> Enables support for NAND controller on MTK SoCs.
>>> This controller is found on mt27xx, mt81xx, mt65xx SoCs.
>>>
>>> +config MTD_NAND_TEGRA
>>> + tristate "Support for NAND on NVIDIA Tegra"
>>> + depends on ARCH_TEGRA
>>> + help
>>> + Enables support for NAND flash on NVIDIA Tegra SoC based boards.
>>> +
>>> endif # MTD_NAND
>>> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
>>> index 165b7ef9e9a1..d5a5f9832b88 100644
>>> --- a/drivers/mtd/nand/raw/Makefile
>>> +++ b/drivers/mtd/nand/raw/Makefile
>>> @@ -56,6 +56,7 @@ obj-$(CONFIG_MTD_NAND_HISI504) += hisi504_nand.o
>>> obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand/
>>> obj-$(CONFIG_MTD_NAND_QCOM) += qcom_nandc.o
>>> obj-$(CONFIG_MTD_NAND_MTK) += mtk_ecc.o mtk_nand.o
>>> +obj-$(CONFIG_MTD_NAND_TEGRA) += tegra_nand.o
>>>
>>> nand-objs := nand_base.o nand_bbt.o nand_timings.o nand_ids.o
>>> nand-objs += nand_amd.o
>>> diff --git a/drivers/mtd/nand/raw/tegra_nand.c b/drivers/mtd/nand/raw/tegra_nand.c
>>> new file mode 100644
>>> index 000000000000..fa236e683fb8
>>> --- /dev/null
>>> +++ b/drivers/mtd/nand/raw/tegra_nand.c
>>> @@ -0,0 +1,915 @@
>>> +/*
>>> + * Copyright (C) 2018 Stefan Agner <stefan@agner.ch>
>>> + * Copyright (C) 2014-2015 Lucas Stach <dev@lynxeye.de>
>>> + * Copyright (C) 2012 Avionic Design GmbH
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/completion.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/dma-mapping.h>
>>> +#include <linux/err.h>
>>> +#include <linux/gpio/consumer.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/io.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mtd/partitions.h>
>>> +#include <linux/mtd/rawnand.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_device.h>> +#include <linux/reset.h>
>>> +
>>> +#define CMD 0x00
>>> +#define CMD_GO (1 << 31)
>>> +#define CMD_CLE (1 << 30)
>>> +#define CMD_ALE (1 << 29)
>>> +#define CMD_PIO (1 << 28)
>>> +#define CMD_TX (1 << 27)
>>> +#define CMD_RX (1 << 26)
>>> +#define CMD_SEC_CMD (1 << 25)
>>> +#define CMD_AFT_DAT (1 << 24)
>>> +#define CMD_TRANS_SIZE(x) (((x - 1) & 0xf) << 20)
>>> +#define CMD_A_VALID (1 << 19)
>>> +#define CMD_B_VALID (1 << 18)
>>> +#define CMD_RD_STATUS_CHK (1 << 17)
>>> +#define CMD_RBSY_CHK (1 << 16)
>>> +#define CMD_CE(x) (1 << (8 + ((x) & 0x7)))
>>> +#define CMD_CLE_SIZE(x) (((x - 1) & 0x3) << 4)
>>> +#define CMD_ALE_SIZE(x) (((x - 1) & 0xf) << 0)
>>> +
>>> +#define STATUS 0x04
>>> +
>>> +#define ISR 0x08
>>> +#define ISR_CORRFAIL_ERR (1 << 24)
>>> +#define ISR_UND (1 << 7)
>>> +#define ISR_OVR (1 << 6)
>>> +#define ISR_CMD_DONE (1 << 5)
>>> +#define ISR_ECC_ERR (1 << 4)
>>> +
>>> +#define IER 0x0c
>>> +#define IER_ERR_TRIG_VAL(x) (((x) & 0xf) << 16)
>>> +#define IER_UND (1 << 7)
>>> +#define IER_OVR (1 << 6)
>>> +#define IER_CMD_DONE (1 << 5)
>>> +#define IER_ECC_ERR (1 << 4)
>>> +#define IER_GIE (1 << 0)
>>> +
>>> +#define CFG 0x10
>>> +#define CFG_HW_ECC (1 << 31)
>>> +#define CFG_ECC_SEL (1 << 30)
>>> +#define CFG_ERR_COR (1 << 29)
>>> +#define CFG_PIPE_EN (1 << 28)
>>> +#define CFG_TVAL_4 (0 << 24)
>>> +#define CFG_TVAL_6 (1 << 24)
>>> +#define CFG_TVAL_8 (2 << 24)
>>> +#define CFG_SKIP_SPARE (1 << 23)
>>> +#define CFG_BUS_WIDTH_8 (0 << 21)
>>> +#define CFG_BUS_WIDTH_16 (1 << 21)
>>> +#define CFG_COM_BSY (1 << 20)
>>> +#define CFG_PS_256 (0 << 16)
>>> +#define CFG_PS_512 (1 << 16)
>>> +#define CFG_PS_1024 (2 << 16)
>>> +#define CFG_PS_2048 (3 << 16)
>>> +#define CFG_PS_4096 (4 << 16)
>>> +#define CFG_SKIP_SPARE_SIZE_4 (0 << 14)
>>> +#define CFG_SKIP_SPARE_SIZE_8 (1 << 14)
>>> +#define CFG_SKIP_SPARE_SIZE_12 (2 << 14)
>>> +#define CFG_SKIP_SPARE_SIZE_16 (3 << 14)
>>> +#define CFG_TAG_BYTE_SIZE(x) ((x) & 0xff)
>>> +
>>> +#define TIMING_1 0x14
>>> +#define TIMING_TRP_RESP(x) (((x) & 0xf) << 28)
>>> +#define TIMING_TWB(x) (((x) & 0xf) << 24)
>>> +#define TIMING_TCR_TAR_TRR(x) (((x) & 0xf) << 20)
>>> +#define TIMING_TWHR(x) (((x) & 0xf) << 16)
>>> +#define TIMING_TCS(x) (((x) & 0x3) << 14)
>>> +#define TIMING_TWH(x) (((x) & 0x3) << 12)
>>> +#define TIMING_TWP(x) (((x) & 0xf) << 8)
>>> +#define TIMING_TRH(x) (((x) & 0xf) << 4)
>>> +#define TIMING_TRP(x) (((x) & 0xf) << 0)
>>> +
>>> +#define RESP 0x18
>>> +
>>> +#define TIMING_2 0x1c
>>> +#define TIMING_TADL(x) ((x) & 0xf)
>>> +
>>> +#define CMD_1 0x20
>>> +#define CMD_2 0x24
>>> +#define ADDR_1 0x28
>>> +#define ADDR_2 0x2c
>>> +
>>> +#define DMA_CTRL 0x30
>>> +#define DMA_CTRL_GO (1 << 31)
>>> +#define DMA_CTRL_IN (0 << 30)
>>> +#define DMA_CTRL_OUT (1 << 30)
>>> +#define DMA_CTRL_PERF_EN (1 << 29)
>>> +#define DMA_CTRL_IE_DONE (1 << 28)
>>> +#define DMA_CTRL_REUSE (1 << 27)
>>> +#define DMA_CTRL_BURST_1 (2 << 24)
>>> +#define DMA_CTRL_BURST_4 (3 << 24)
>>> +#define DMA_CTRL_BURST_8 (4 << 24)
>>> +#define DMA_CTRL_BURST_16 (5 << 24)
>>> +#define DMA_CTRL_IS_DONE (1 << 20)
>>> +#define DMA_CTRL_EN_A (1 << 2)
>>> +#define DMA_CTRL_EN_B (1 << 1)
>>> +
>>> +#define DMA_CFG_A 0x34
>>> +#define DMA_CFG_B 0x38
>>> +
>>> +#define FIFO_CTRL 0x3c
>>> +#define FIFO_CTRL_CLR_ALL (1 << 3)
>>> +
>>> +#define DATA_PTR 0x40
>>> +#define TAG_PTR 0x44
>>> +#define ECC_PTR 0x48
>>> +
>>> +#define DEC_STATUS 0x4c
>>> +#define DEC_STATUS_A_ECC_FAIL (1 << 1)
>>> +#define DEC_STATUS_ERR_COUNT_MASK 0x00ff0000
>>> +#define DEC_STATUS_ERR_COUNT_SHIFT 16
>>> +
>>> +#define HWSTATUS_CMD 0x50
>>> +#define HWSTATUS_MASK 0x54
>>> +#define HWSTATUS_RDSTATUS_MASK(x) (((x) & 0xff) << 24)
>>> +#define HWSTATUS_RDSTATUS_VALUE(x) (((x) & 0xff) << 16)
>>> +#define HWSTATUS_RBSY_MASK(x) (((x) & 0xff) << 8)
>>> +#define HWSTATUS_RBSY_VALUE(x) (((x) & 0xff) << 0)
>>> +
>>> +#define DEC_STAT_RESULT 0xd0
>>> +#define DEC_STAT_BUF 0xd4
>>> +#define DEC_STAT_BUF_CORR_SEC_FLAG_MASK 0x00ff0000
>>> +#define DEC_STAT_BUF_CORR_SEC_FLAG_SHIFT 16
>>> +#define DEC_STAT_BUF_MAX_CORR_CNT_MASK 0x00001f00
>>> +#define DEC_STAT_BUF_MAX_CORR_CNT_SHIFT 8
>>> +
>>> +struct tegra_nand {
>>> + void __iomem *regs;
>>> + struct clk *clk;
>>> + struct gpio_desc *wp_gpio;
>>> +
>>> + struct nand_chip chip;
>>> + struct device *dev;
>>> +
>>> + struct completion command_complete;
>>> + struct completion dma_complete;
>>> + bool last_read_error;
>>> +
>>> + dma_addr_t data_dma;
>>> + void *data_buf;
>>> + dma_addr_t oob_dma;
>>> + void *oob_buf;
>>> +
>>> + int cur_chip;
>>> +};
>>> +
>>> +static inline struct tegra_nand *to_tegra_nand(struct mtd_info *mtd)
>>> +{
>>> + struct nand_chip *chip = mtd_to_nand(mtd);
>>> +
>>> + return nand_get_controller_data(chip);
>>> +}
>>> +
>>> +static int tegra_nand_ooblayout_16_ecc(struct mtd_info *mtd, int section,
>>> + struct mtd_oob_region *oobregion)
>>> +{
>>> + if (section > 0)
>>> + return -ERANGE;
>>> +
>>> + oobregion->offset = 4;
>>> + oobregion->length = 4;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int tegra_nand_ooblayout_16_free(struct mtd_info *mtd, int section,
>>> + struct mtd_oob_region *oobregion)
>>> +{
>>> + if (section > 0)
>>> + return -ERANGE;
>>> +
>>> + oobregion->offset = 8;
>>> + oobregion->length = 8;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct mtd_ooblayout_ops tegra_nand_oob_16_ops = {
>>> + .ecc = tegra_nand_ooblayout_16_ecc,
>>> + .free = tegra_nand_ooblayout_16_free,
>>> +};
>>> +
>>> +static int tegra_nand_ooblayout_64_ecc(struct mtd_info *mtd, int section,
>>> + struct mtd_oob_region *oobregion)
>>> +{
>>> + if (section > 0)
>>> + return -ERANGE;
>>> +
>>> + oobregion->offset = 4;
>>> + oobregion->length = 36;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int tegra_nand_ooblayout_64_free(struct mtd_info *mtd, int section,
>>> + struct mtd_oob_region *oobregion)
>>> +{
>>> + if (section > 0)
>>> + return -ERANGE;
>>> +
>>> + oobregion->offset = 40;
>>> + oobregion->length = 24;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct mtd_ooblayout_ops tegra_nand_oob_64_ops = {
>>> + .ecc = tegra_nand_ooblayout_64_ecc,
>>> + .free = tegra_nand_ooblayout_64_free,
>>> +};
>>> +
>>> +static int tegra_nand_ooblayout_128_ecc(struct mtd_info *mtd, int section,
>>> + struct mtd_oob_region *oobregion)
>>> +{
>>> + if (section > 0)
>>> + return -ERANGE;
>>> +
>>> + oobregion->offset = 4;
>>> + oobregion->length = 72;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int tegra_nand_ooblayout_128_free(struct mtd_info *mtd, int section,
>>> + struct mtd_oob_region *oobregion)
>>> +{
>>> + if (section > 0)
>>> + return -ERANGE;
>>> +
>>> + oobregion->offset = 76;
>>> + oobregion->length = 52;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct mtd_ooblayout_ops tegra_nand_oob_128_ops = {
>>> + .ecc = tegra_nand_ooblayout_128_ecc,
>>> + .free = tegra_nand_ooblayout_128_free,
>>> +};
>>> +
>>> +static int tegra_nand_ooblayout_224_ecc(struct mtd_info *mtd, int section,
>>> + struct mtd_oob_region *oobregion)
>>> +{
>>> + if (section > 0)
>>> + return -ERANGE;
>>> +
>>> + oobregion->offset = 4;
>>> + oobregion->length = 144;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int tegra_nand_ooblayout_224_free(struct mtd_info *mtd, int section,
>>> + struct mtd_oob_region *oobregion)
>>> +{
>>> + if (section > 0)
>>> + return -ERANGE;
>>> +
>>> + oobregion->offset = 148;
>>> + oobregion->length = 76;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct mtd_ooblayout_ops tegra_nand_oob_224_ops = {
>>> + .ecc = tegra_nand_ooblayout_224_ecc,
>>> + .free = tegra_nand_ooblayout_224_free,
>>> +};
>>> +
>>> +static irqreturn_t tegra_nand_irq(int irq, void *data)
>>> +{
>>> + struct tegra_nand *nand = data;
>>> + u32 isr, dma;
>>> +
>>> + isr = readl(nand->regs + ISR);
>>> + dma = readl(nand->regs + DMA_CTRL);
>>
>> You could use readl_relaxed() here.
>>
>>> + dev_dbg(nand->dev, "isr %08x\n", isr);
>>> +
>>> + if (!isr && !(dma & DMA_CTRL_IS_DONE))
>>> + return IRQ_NONE;
>>> +
>>> + if (isr & ISR_CORRFAIL_ERR)
>>> + nand->last_read_error = true;
>>> +
>>> + if (isr & ISR_CMD_DONE)
>>> + complete(&nand->command_complete);
>>> +
>>> + if (isr & ISR_UND)
>>> + dev_dbg(nand->dev, "FIFO underrun\n");
>>> +
>>> + if (isr & ISR_OVR)
>>> + dev_dbg(nand->dev, "FIFO overrun\n");
>>> +
>>> + /* handle DMA interrupts */
>>> + if (dma & DMA_CTRL_IS_DONE) {
>>> + writel(dma, nand->regs + DMA_CTRL);
>>> + complete(&nand->dma_complete);
>>> + }
>>> +
>>> + /* clear interrupts */
>>> + writel(isr, nand->regs + ISR);
>>> +
>>> + return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int tegra_nand_cmd(struct nand_chip *chip,
>>> + const struct nand_subop *subop)
>>> +{
>>> + const struct nand_op_instr *instr;
>>> + const struct nand_op_instr *instr_data_in = NULL;
>>> + struct mtd_info *mtd = nand_to_mtd(chip);
>>> + struct tegra_nand *nand = to_tegra_nand(mtd);
>>> + unsigned int op_id = -1, trfr_in_sz = 0, trfr_out_sz = 0, offset = 0;
>>> + bool first_cmd = true;
>>> + bool force8bit;
>>
>> The force8bit variable isn't used anywhere in the code, so compiler should warn
>> you about the "unused variable", is it the case?
>>
>>> + u32 cmd = 0;
>>> + u32 value;
>>> +
>>
>> The op_id=-1 above is probably because the loop below was:
>>
>> while (++op_id < subop->ninstrs) {}
>>
>> Both variants are fine, but the for-loop is a bit more explicit. You could omit
>> the above op_id variable initialization for consistency.
>>
>
> Agreed, will do the for loop variant.
>
>>> + for (op_id = 0; op_id < subop->ninstrs; op_id++) {
>>> + unsigned int naddrs, i;
>>> + const u8 *addrs;
>>> + u32 addr1 = 0, addr2 = 0;
>>> +
>>> + instr = &subop->instrs[op_id];
>>> +
>>> + switch (instr->type) {
>>> + case NAND_OP_CMD_INSTR:
>>> + if (first_cmd) {
>>> + cmd |= CMD_CLE;
>>> + writel(instr->ctx.cmd.opcode, nand->regs + CMD_1);
>>> + } else {
>>> + cmd |= CMD_SEC_CMD;
>>> + writel(instr->ctx.cmd.opcode, nand->regs + CMD_2);
>>> + }
>>> + first_cmd = false;
>>> + break;
>>> + case NAND_OP_ADDR_INSTR:
>>> + offset = nand_subop_get_addr_start_off(subop, op_id);
>>> + naddrs = nand_subop_get_num_addr_cyc(subop, op_id);
>>> + addrs = &instr->ctx.addr.addrs[offset];
>>> +
>>> + cmd |= CMD_ALE | CMD_ALE_SIZE(naddrs);
>>> + for (i = 0; i < min_t(unsigned int, 4, naddrs); i++)
>>> + addr1 |= *addrs++ << (8 * i);
>>> + naddrs -= i;
>>> + for (i = 0; i < min_t(unsigned int, 4, naddrs); i++)
>>> + addr2 |= *addrs++ << (8 * i);
>>> + writel(addr1, nand->regs + ADDR_1);
>>> + writel(addr2, nand->regs + ADDR_2);
>>> + break;
>>> +
>>> + case NAND_OP_DATA_IN_INSTR:
>>> + trfr_in_sz = nand_subop_get_data_len(subop, op_id);
>>> + offset = nand_subop_get_data_start_off(subop, op_id);
>>> +
>>> + cmd |= CMD_TRANS_SIZE(trfr_in_sz) | CMD_PIO | CMD_RX | CMD_A_VALID;
>>> +
>>> + instr_data_in = instr;
>>> + break;
>>> +
>>> + case NAND_OP_DATA_OUT_INSTR:
>>> + trfr_out_sz = nand_subop_get_data_len(subop, op_id);
>>> + offset = nand_subop_get_data_start_off(subop, op_id);
>>> + trfr_out_sz = min_t(size_t, trfr_out_sz, 4);
>>> +
>>> + cmd |= CMD_TRANS_SIZE(trfr_out_sz) | CMD_PIO | CMD_TX | CMD_A_VALID;
>>> +
>>> + memcpy(&value, instr->ctx.data.buf.out + offset, trfr_out_sz)> + writel(value, nand->regs + RESP);
>>
>> Note:
>>
>> The memcpy + readl / writel won't work with a big-endian kernel,
>> cpu_to_[bl]e32() should be applied in a such cases.
>>
>> Tegra's I2C driver had a similar memcpy-case issue, it was corrected during the
>> attempt to get BE kernel support for Tegra. But that attempt was quite long time
>> ago and Tegra maintainers were not very excited to have to do more testing work
>> for each kernel release and hence Tegra is LE-only as of today.
>>
>>> +
>>> + break;
>>> + case NAND_OP_WAITRDY_INSTR:
>>> + cmd |= CMD_RBSY_CHK;
>>> + break;
>>> +
>>> + }
>>> + }
>>> +
>>> +
>>> + cmd |= CMD_GO | CMD_CE(nand->cur_chip);
>>> + writel(cmd, nand->regs + CMD);
>>> + wait_for_completion(&nand->command_complete);
>>> +
>>> + if (instr_data_in) {
>>> + u32 value;
>>> + size_t n = min_t(size_t, trfr_in_sz, 4);
>>> +
>>> + value = readl(nand->regs + RESP);
>>> + memcpy(instr_data_in->ctx.data.buf.in + offset, &value, n);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct nand_op_parser tegra_nand_op_parser = NAND_OP_PARSER(
>>> + NAND_OP_PARSER_PATTERN(tegra_nand_cmd,
>>> + NAND_OP_PARSER_PAT_CMD_ELEM(true),
>>> + NAND_OP_PARSER_PAT_ADDR_ELEM(true, 8),
>>> + NAND_OP_PARSER_PAT_CMD_ELEM(true),
>>> + NAND_OP_PARSER_PAT_WAITRDY_ELEM(true)),
>>> + NAND_OP_PARSER_PATTERN(tegra_nand_cmd,
>>> + NAND_OP_PARSER_PAT_DATA_OUT_ELEM(false, 4)),
>>> + NAND_OP_PARSER_PATTERN(tegra_nand_cmd,
>>> + NAND_OP_PARSER_PAT_CMD_ELEM(true),
>>> + NAND_OP_PARSER_PAT_ADDR_ELEM(true, 8),
>>> + NAND_OP_PARSER_PAT_CMD_ELEM(true),
>>> + NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
>>> + NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, 4)),
>>> + );
>>> +
>>> +static int tegra_nand_exec_op(struct nand_chip *chip,
>>> + const struct nand_operation *op,
>>> + bool check_only)
>>> +{
>>> + return nand_op_parser_exec_op(chip, &tegra_nand_op_parser, op,
>>> + check_only);
>>> +}
>>> +static void tegra_nand_select_chip(struct mtd_info *mtd, int chip)
>>> +{
>>> + struct tegra_nand *nand = to_tegra_nand(mtd);
>>> +
>>> + nand->cur_chip = chip;
>>> +}
>>> +
>>> +static u32 tegra_nand_fill_address(struct mtd_info *mtd, struct nand_chip *chip,
>>> + int page)
>>> +{
>>> + struct tegra_nand *nand = to_tegra_nand(mtd);
>>> +
>>> + /* Lower 16-bits are column, always 0 */
>>> + writel(page << 16, nand->regs + ADDR_1);
>>> +
>>> + if (chip->options & NAND_ROW_ADDR_3) {
>>> + writel(page >> 16, nand->regs + ADDR_2);
>>> + return 5;
>>> + }
>>> +
>>> + return 4;
>>> +}
>>> +
>>> +static int tegra_nand_read_page(struct mtd_info *mtd, struct nand_chip *chip,
>>> + uint8_t *buf, int oob_required, int page)
>>> +{
>>> + struct tegra_nand *nand = to_tegra_nand(mtd);
>>> + u32 value, addrs;
>>> +
>>> + writel(NAND_CMD_READ0, nand->regs + CMD_1);
>>> + writel(NAND_CMD_READSTART, nand->regs + CMD_2);
>>> +
>>> + addrs = tegra_nand_fill_address(mtd, chip, page);
>>> +
>>> + value = readl(nand->regs + CFG);
>>> + value |= CFG_HW_ECC | CFG_ERR_COR;
>>> + writel(value, nand->regs + CFG);
>>> +
>>> + writel(mtd->writesize - 1, nand->regs + DMA_CFG_A);
>>> + writel(nand->data_dma, nand->regs + DATA_PTR);
>>> +
>>> + if (oob_required) {
>>> + writel(mtd_ooblayout_count_freebytes(mtd) - 1,
>>> + nand->regs + DMA_CFG_B);
>>> + writel(nand->oob_dma, nand->regs + TAG_PTR);
>>> + } else {
>>> + writel(0, nand->regs + DMA_CFG_B);
>>> + writel(0, nand->regs + TAG_PTR);
>>> + }
>>> +
>>> + value = DMA_CTRL_GO | DMA_CTRL_IN | DMA_CTRL_PERF_EN |
>>> + DMA_CTRL_REUSE | DMA_CTRL_IE_DONE | DMA_CTRL_IS_DONE |
>>> + DMA_CTRL_BURST_8 | DMA_CTRL_EN_A;
>>
>> Wouldn't be more efficient to set DMA burst to 16 words? The writesize seems
>> always aligned to at least 64 words.
>>
>
> Hm, haven't tested 16 words, 8 was the setting Lucas used.
>
> Are you sure this is only about write size? Not sure, but isn't the ECC
> area also DMA'd? On Colibri we use RS with t=8, hence 144 bytes parity,
> so this would be properly aligned non the less...
>
I don't know, that's up to you to figure out. Is RS stands for Reed-Solomon and
t=8 is the max number of redundant words?
>>> + if (oob_required)
>>> + value |= DMA_CTRL_EN_B;
>>> + writel(value, nand->regs + DMA_CTRL);
>>> +
>>> + value = CMD_CLE | CMD_ALE | CMD_ALE_SIZE(addrs) | CMD_SEC_CMD |
>>> + CMD_RBSY_CHK | CMD_GO | CMD_RX | CMD_TRANS_SIZE(9) |
>>> + CMD_A_VALID | CMD_CE(nand->cur_chip);
>>> + if (oob_required)
>>> + value |= CMD_B_VALID;
>>> + writel(value, nand->regs + CMD);
>>> +
>>> + wait_for_completion(&nand->command_complete);
>>> + wait_for_completion(&nand->dma_complete);
>>> +
>>> + if (oob_required) {
>>> + struct mtd_oob_region oobregion;
>>> +
>>> + mtd_ooblayout_free(mtd, 0, &oobregion);
>>> + memcpy(chip->oob_poi, nand->oob_buf + oobregion.offset,
>>> + mtd_ooblayout_count_freebytes(mtd));
>>> + }
>>> + memcpy(buf, nand->data_buf, mtd->writesize);
>>> +
>>> + value = readl(nand->regs + CFG);
>>> + value &= ~(CFG_HW_ECC | CFG_ERR_COR);
>>> + writel(value, nand->regs + CFG);
>>> +
>>> + value = readl(nand->regs + DEC_STATUS);
>>> + if (value & DEC_STATUS_A_ECC_FAIL) {
>>> + /*
>>> + * The ECC isn't smart enough to figure out if a page is
>>> + * completely erased and flags an error in this case. So we
>>> + * check the read data here to figure out if it's a legitimate
>>> + * error or a false positive.
>>> + */
>>> + int i, err;
>>> + int flips_threshold = chip->ecc.strength / 2;
>>> + int max_bitflips = 0;
>>> +
>>> + for (i = 0; i < chip->ecc.steps; i++) {
>>> + u8 *data = buf + (chip->ecc.size * i);
>>> + err = nand_check_erased_ecc_chunk(data, chip->ecc.size,
>>> + NULL, 0,
>>> + NULL, 0,
>>> + flips_threshold);
>>> + if (err < 0)
>>> + return err;
>>> +
>>> + max_bitflips += max_bitflips;
>>> + }
>>> +
>>> + return max_bitflips;
>>> + }
>>> +
>>> + if (nand->last_read_error) {
>>> + int max_corr_cnt, corr_sec_flag;
>>> +
>>> + value = readl(nand->regs + DEC_STAT_BUF);
>>> + corr_sec_flag = (value & DEC_STAT_BUF_CORR_SEC_FLAG_MASK) >>
>>> + DEC_STAT_BUF_CORR_SEC_FLAG_SHIFT;
>>> + max_corr_cnt = (value & DEC_STAT_BUF_MAX_CORR_CNT_MASK) >>
>>> + DEC_STAT_BUF_MAX_CORR_CNT_SHIFT;
>>> +
>>> + /*
>>> + * The value returned in the register is the maximum of
>>> + * bitflips encountered in any of the ECC regions. As there is
>>> + * no way to get the number of bitflips in a specific regions
>>> + * we are not able to deliver correct stats but instead
>>> + * overestimate the number of corrected bitflips by assuming
>>> + * that all regions where errors have been corrected
>>> + * encountered the maximum number of bitflips.
>>> + */
>>> + mtd->ecc_stats.corrected += max_corr_cnt * hweight8(corr_sec_flag);
>>> + nand->last_read_error = false;
>>> + return value;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int tegra_nand_write_page(struct mtd_info *mtd, struct nand_chip *chip,
>>> + const uint8_t *buf, int oob_required, int page)
>>> +{
>>> + struct tegra_nand *nand = to_tegra_nand(mtd);
>>> + u32 value, addrs;
>>> +
>>> + writel(NAND_CMD_SEQIN, nand->regs + CMD_1);
>>> + writel(NAND_CMD_PAGEPROG, nand->regs + CMD_2);
>>> +
>>> + addrs = tegra_nand_fill_address(mtd, chip, page);
>>> +
>>> + value = readl(nand->regs + CFG);
>>> + value |= CFG_HW_ECC | CFG_ERR_COR;
>>> + writel(value, nand->regs + CFG);
>>> +
>>> + memcpy(nand->data_buf, buf, mtd->writesize);
>>> +
>>> + writel(mtd->writesize - 1, nand->regs + DMA_CFG_A);
>>> + writel(nand->data_dma, nand->regs + DATA_PTR);
>>> +
>>> + if (oob_required) {
>>> + struct mtd_oob_region oobregion;
>>> +
>>> + mtd_ooblayout_free(mtd, 0, &oobregion);
>>> + memcpy(nand->oob_buf, chip->oob_poi + oobregion.offset,
>>> + mtd_ooblayout_count_freebytes(mtd));
>>> + writel(mtd_ooblayout_count_freebytes(mtd) - 1,
>>> + nand->regs + DMA_CFG_B);
>>> + writel(nand->oob_dma, nand->regs + TAG_PTR);
>>> + } else {
>>> + writel(0, nand->regs + DMA_CFG_B);
>>> + writel(0, nand->regs + TAG_PTR);
>>> + }
>>> +
>>> + value = DMA_CTRL_GO | DMA_CTRL_OUT | DMA_CTRL_PERF_EN |
>>> + DMA_CTRL_IE_DONE | DMA_CTRL_IS_DONE |
>>> + DMA_CTRL_BURST_8 | DMA_CTRL_EN_A;
>>> + if (oob_required)
>>> + value |= DMA_CTRL_EN_B;
>>> + writel(value, nand->regs + DMA_CTRL);
>>> +
>>> + value = CMD_CLE | CMD_ALE | CMD_ALE_SIZE(addrs) | CMD_SEC_CMD |
>>> + CMD_AFT_DAT | CMD_RBSY_CHK | CMD_GO | CMD_TX | CMD_A_VALID |
>>> + CMD_TRANS_SIZE(9) | CMD_CE(nand->cur_chip);
>>> + if (oob_required)
>>> + value |= CMD_B_VALID;
>>> + writel(value, nand->regs + CMD);
>>> +
>>> + wait_for_completion(&nand->command_complete);
>>> + wait_for_completion(&nand->dma_complete);
>>> +
>>> + value = readl(nand->regs + CFG);
>>> + value &= ~(CFG_HW_ECC | CFG_ERR_COR);
>>> + writel(value, nand->regs + CFG);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void tegra_nand_setup_timing(struct tegra_nand *nand, int mode)
>>> +{
>>> + /*
>>> + * The period (and all other timings in this function) is in ps,
>>> + * so need to take care here to avoid integer overflows.
>>> + */
>>> + unsigned int rate = clk_get_rate(nand->clk) / 1000000;
>>> + unsigned int period = DIV_ROUND_UP(1000000, rate);
>>> + const struct nand_sdr_timings *timings;
>>> + u32 val, reg = 0;
>>> +
>>> + timings = onfi_async_timing_mode_to_sdr_timings(mode);
>>> +
>>> + val = DIV_ROUND_UP(max3(timings->tAR_min, timings->tRR_min,
>>> + timings->tRC_min), period);
>>> + if (val > 2)
>>> + val -= 3;
>>> + reg |= TIMING_TCR_TAR_TRR(val);
>>> +
>>> + val = DIV_ROUND_UP(max(max(timings->tCS_min, timings->tCH_min),
>>> + max(timings->tALS_min, timings->tALH_min)),
>>> + period);
>>> + if (val > 1)
>>> + val -= 2;
>>> + reg |= TIMING_TCS(val);
>>> +
>>> + val = DIV_ROUND_UP(max(timings->tRP_min, timings->tREA_max) + 6000,
>>> + period);
>>> + reg |= TIMING_TRP(val) | TIMING_TRP_RESP(val);
>>> +
>>> + reg |= TIMING_TWB(DIV_ROUND_UP(timings->tWB_max, period));
>>> + reg |= TIMING_TWHR(DIV_ROUND_UP(timings->tWHR_min, period));
>>> + reg |= TIMING_TWH(DIV_ROUND_UP(timings->tWH_min, period));
>>> + reg |= TIMING_TWP(DIV_ROUND_UP(timings->tWP_min, period));
>>> + reg |= TIMING_TRH(DIV_ROUND_UP(timings->tRHW_min, period));
>>> +
>>> + writel(reg, nand->regs + TIMING_1);
>>> +
>>> + val = DIV_ROUND_UP(timings->tADL_min, period);
>>> + if (val > 2)
>>> + val -= 3;
>>> + reg = TIMING_TADL(val);
>>> +
>>> + writel(reg, nand->regs + TIMING_2);
>>> +}
>>> +
>>> +static void tegra_nand_setup_chiptiming(struct tegra_nand *nand)
>>> +{
>>> + struct nand_chip *chip = &nand->chip;
>>> + int mode;
>>> +
>>> + mode = onfi_get_async_timing_mode(chip);
>>> + if (mode == ONFI_TIMING_MODE_UNKNOWN)
>>> + mode = chip->onfi_timing_mode_default;
>>> + else
>>> + mode = fls(mode);
>>> +
>>> + tegra_nand_setup_timing(nand, mode);
>>> +}
>>> +
>>> +static int tegra_nand_probe(struct platform_device *pdev)
>>> +{
>>> + struct reset_control *rst;
>>> + struct tegra_nand *nand;
>>> + struct nand_chip *chip;
>>> + struct mtd_info *mtd;
>>> + struct resource *res;
>>> + unsigned long value;
>>> + int irq, err = 0;
>>> +
>>> + nand = devm_kzalloc(&pdev->dev, sizeof(*nand), GFP_KERNEL);
>>> + if (!nand)
>>> + return -ENOMEM;
>>> +
>>> + nand->dev = &pdev->dev;
>>> +
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + nand->regs = devm_ioremap_resource(&pdev->dev, res);
>>> + if (IS_ERR(nand->regs))
>>> + return PTR_ERR(nand->regs);
>>> +
>>> + irq = platform_get_irq(pdev, 0);
>>> + err = devm_request_irq(&pdev->dev, irq, tegra_nand_irq, 0,
>>> + dev_name(&pdev->dev), nand);
>>> + if (err)
>>> + return err;
>>> +
>>> + rst = devm_reset_control_get(&pdev->dev, "nand");
>>> + if (IS_ERR(rst))
>>> + return PTR_ERR(rst);
>>> +
>>> + nand->clk = devm_clk_get(&pdev->dev, "nand");
>>> + if (IS_ERR(nand->clk))
>>> + return PTR_ERR(nand->clk);
>>> +
>>> + nand->wp_gpio = gpiod_get_optional(&pdev->dev, "wp-gpios",
>>> + GPIOD_OUT_HIGH);
>>> + if (IS_ERR(nand->wp_gpio))
>>> + return PTR_ERR(nand->wp_gpio);
>>> +
>>> + err = clk_prepare_enable(nand->clk);
>>> + if (err)
>>> + return err;
>>> +
>>> + reset_control_assert(rst);
>>> + udelay(2);
>>> + reset_control_deassert(rst);
>>
>> You could use the reset_control_reset() here, though it uses the 1 usec delay
>> instead of 2, but I think it shouldn't really matter.
>>
>>> +
>>> + value = HWSTATUS_RDSTATUS_MASK(1) | HWSTATUS_RDSTATUS_VALUE(0) |
>>> + HWSTATUS_RBSY_MASK(NAND_STATUS_READY) |
>>> + HWSTATUS_RBSY_VALUE(NAND_STATUS_READY);
>>> + writel(NAND_CMD_STATUS, nand->regs + HWSTATUS_CMD);
>>> + writel(value, nand->regs + HWSTATUS_MASK);
>>> +
>>> + init_completion(&nand->command_complete);
>>> + init_completion(&nand->dma_complete);
>>> +
>>> + /* clear interrupts */
>>> + value = readl(nand->regs + ISR);
>>> + writel(value, nand->regs + ISR);
>>
>> Isn't a set ISR bit means that NAND HW asserts interrupt line? If yes, then you
>> may want to move devm_request_irq() after resetting the HW state, to be on a
>> safe side.
>>
>
> Agreed.
>
>>> +
>>> + writel(DMA_CTRL_IS_DONE, nand->regs + DMA_CTRL);
>>> +
>>> + /* enable interrupts */
>>> + value = IER_UND | IER_OVR | IER_CMD_DONE | IER_ECC_ERR | IER_GIE;
>>> + writel(value, nand->regs + IER);
>>> +
>>> + /* reset config */
>>> + writel(0, nand->regs + CFG);
>>
>> Is this "reset config" really needed? It is overwritten below in the code.
>>
>
> We already use the NAND peripheral in nand_scan_ident, and then properly
> set it up later. I think it is better to have a known config state
> during this phase...
>
>>
>> Also, maybe you could factor out the HW reset/init stuff into a respective
>> tegra_nand_hw_reset() / tegra_nand_hw_init(), just for consistency.
>>
>
> I will consider that.
>
> Thanks for your review!
>
> --
> Stefan
>
>>> +
>>> + chip = &nand->chip;
>>> + mtd = nand_to_mtd(chip);
>>> +
>>> + mtd->dev.parent = &pdev->dev;
>>> + mtd->name = "tegra_nand";
>>> + mtd->owner = THIS_MODULE;
>>> +
>>> + nand_set_flash_node(chip, pdev->dev.of_node);
>>> + nand_set_controller_data(chip, nand);
>>> +
>>> + chip->options = NAND_NO_SUBPAGE_WRITE;
>>> + chip->exec_op = tegra_nand_exec_op;
>>> + chip->select_chip = tegra_nand_select_chip;
>>> + tegra_nand_setup_timing(nand, 0);
>>> +
>>> + err = nand_scan_ident(mtd, 1, NULL);
>>> + if (err)
>>> + goto err_disable_clk;
>>> +
>>> + if (chip->bbt_options & NAND_BBT_USE_FLASH)
>>> + chip->bbt_options |= NAND_BBT_NO_OOB;
>>> +
>>> + nand->data_buf = dmam_alloc_coherent(&pdev->dev, mtd->writesize,
>>> + &nand->data_dma, GFP_KERNEL);
>>> + if (!nand->data_buf) {
>>> + err = -ENOMEM;
>>> + goto err_disable_clk;
>>> + }
>>> +
>>> + nand->oob_buf = dmam_alloc_coherent(&pdev->dev, mtd->oobsize,
>>> + &nand->oob_dma, GFP_KERNEL);
>>> + if (!nand->oob_buf) {
>>> + err = -ENOMEM;
>>> + goto err_disable_clk;
>>> + }
>>> +
>>> + chip->ecc.mode = NAND_ECC_HW;
>>> + chip->ecc.size = 512;
>>> + chip->ecc.read_page = tegra_nand_read_page;
>>> + chip->ecc.write_page = tegra_nand_write_page;
>>> +
>>> + value = readl(nand->regs + CFG);
>>> + value |= CFG_PIPE_EN | CFG_SKIP_SPARE | CFG_SKIP_SPARE_SIZE_4 |
>>> + CFG_TAG_BYTE_SIZE(mtd_ooblayout_count_freebytes(mtd) - 1);
>>> +
>>> + if (chip->options & NAND_BUSWIDTH_16)
>>> + value |= CFG_BUS_WIDTH_16;
>>> +
>>> + switch (mtd->oobsize) {
>>> + case 16:
>>> + mtd_set_ooblayout(mtd, &tegra_nand_oob_16_ops);
>>> + chip->ecc.strength = 1;
>>> + chip->ecc.bytes = 4;
>>> + break;
>>> + case 64:
>>> + mtd_set_ooblayout(mtd, &tegra_nand_oob_64_ops);
>>> + chip->ecc.strength = 8;
>>> + chip->ecc.bytes = 18;
>>> + value |= CFG_ECC_SEL | CFG_TVAL_8;
>>> + break;
>>> + case 128:
>>> + mtd_set_ooblayout(mtd, &tegra_nand_oob_128_ops);
>>> + chip->ecc.strength = 8;
>>> + chip->ecc.bytes = 18;
>>> + value |= CFG_ECC_SEL | CFG_TVAL_8;
>>> + break;
>>> + case 224:
>>> + mtd_set_ooblayout(mtd, &tegra_nand_oob_224_ops);
>>> + chip->ecc.strength = 8;
>>> + chip->ecc.bytes = 18;
>>> + value |= CFG_ECC_SEL | CFG_TVAL_8;
>>> + break;
>>> + default:
>>> + dev_err(&pdev->dev, "unhandled OOB size %d\n", mtd->oobsize);
>>> + err = -ENODEV;
>>> + goto err_disable_clk;
>>> + }
>>> +
>>> + switch (mtd->writesize) {
>>> + case 256:
>>> + value |= CFG_PS_256;
>>> + break;
>>> + case 512:
>>> + value |= CFG_PS_512;
>>> + break;
>>> + case 1024:
>>> + value |= CFG_PS_1024;
>>> + break;
>>> + case 2048:
>>> + value |= CFG_PS_2048;
>>> + break;
>>> + case 4096:
>>> + value |= CFG_PS_4096;
>>> + break;
>>> + default:
>>> + dev_err(&pdev->dev, "unhandled writesize %d\n", mtd->writesize);
>>> + err = -ENODEV;
>>> + goto err_disable_clk;
>>> + }
>>> +
>>> + writel(value, nand->regs + CFG);
>>> +
>>> + tegra_nand_setup_chiptiming(nand);
>>> +
>>> + err = nand_scan_tail(mtd);
>>> + if (err)
>>> + goto err_disable_clk;
>>> +
>>> + err = mtd_device_register(mtd, NULL, 0);
>>> + if (err)
>>> + goto err_cleanup_nand;
>>> +
>>> + platform_set_drvdata(pdev, nand);
>>> +
>>> + return 0;
>>> +
>>> +err_cleanup_nand:
>>> + nand_cleanup(chip);
>>> +err_disable_clk:
>>> + clk_disable_unprepare(nand->clk);
>>> + return err;
>>> +}
>>> +
>>> +static int tegra_nand_remove(struct platform_device *pdev)
>>> +{
>>> + struct tegra_nand *nand = platform_get_drvdata(pdev);
>>> +
>>> + nand_release(nand_to_mtd(&nand->chip));
>>> +
>>> + clk_disable_unprepare(nand->clk);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct of_device_id tegra_nand_of_match[] = {
>>> + { .compatible = "nvidia,tegra20-nand" },
>>> + { /* sentinel */ }
>>> +};
>>> +
>>> +static struct platform_driver tegra_nand_driver = {
>>> + .driver = {
>>> + .name = "tegra-nand",
>>> + .of_match_table = tegra_nand_of_match,
>>> + },
>>> + .probe = tegra_nand_probe,
>>> + .remove = tegra_nand_remove,
>>> +};
>>> +module_platform_driver(tegra_nand_driver);
>>> +
>>> +MODULE_DESCRIPTION("NVIDIA Tegra NAND driver");
>>> +MODULE_AUTHOR("Thierry Reding <thierry.reding@nvidia.com>");
>>> +MODULE_AUTHOR("Lucas Stach <dev@lynxeye.de>");
>>> +MODULE_AUTHOR("Stefan Agner <stefan@agner.ch>");
>>> +MODULE_LICENSE("GPL v2");
>>> +MODULE_DEVICE_TABLE(of, tegra_nand_of_match);
>>>
^ permalink raw reply
* Re: [PATCH RFC V2 2/6] hwmon: Add support for RPi voltage sensor
From: Guenter Roeck @ 2018-05-22 13:41 UTC (permalink / raw)
To: Stefan Wahren, Rob Herring, Mark Rutland, Jean Delvare,
Jonathan Corbet, Eric Anholt
Cc: linux-hwmon, devicetree, Florian Fainelli, Scott Branden,
linux-doc, Ray Jui, Phil Elwell, Noralf Trønnes,
bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel
In-Reply-To: <1526988112-4021-3-git-send-email-stefan.wahren@i2se.com>
On 05/22/2018 04:21 AM, Stefan Wahren wrote:
> Currently there is no easy way to detect undervoltage conditions on a
> remote Raspberry Pi. This hwmon driver retrieves the state of the
> undervoltage sensor via mailbox interface. The handling based on
> Noralf's modifications to the downstream firmware driver. In case of
> an undervoltage condition only an entry is written to the kernel log.
>
> CC: "Noralf Trønnes" <noralf@tronnes.org>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
> Documentation/hwmon/raspberrypi-hwmon | 22 +++++
> drivers/hwmon/Kconfig | 10 ++
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/raspberrypi-hwmon.c | 168 ++++++++++++++++++++++++++++++++++
> 4 files changed, 201 insertions(+)
> create mode 100644 Documentation/hwmon/raspberrypi-hwmon
> create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
>
> diff --git a/Documentation/hwmon/raspberrypi-hwmon b/Documentation/hwmon/raspberrypi-hwmon
> new file mode 100644
> index 0000000..3c92e2c
> --- /dev/null
> +++ b/Documentation/hwmon/raspberrypi-hwmon
> @@ -0,0 +1,22 @@
> +Kernel driver raspberrypi-hwmon
> +===============================
> +
> +Supported boards:
> + * Raspberry Pi A+ (via GPIO on SoC)
> + * Raspberry Pi B+ (via GPIO on SoC)
> + * Raspberry Pi 2 B (via GPIO on SoC)
> + * Raspberry Pi 3 B (via GPIO on port expander)
> + * Raspberry Pi 3 B+ (via PMIC)
> +
> +Author: Stefan Wahren <stefan.wahren@i2se.com>
> +
> +Description
> +-----------
> +
> +This driver periodically polls a mailbox property of the VC4 firmware to detect
> +undervoltage conditions.
> +
> +Sysfs entries
> +-------------
> +
> +in0_lcrit_alarm Undervoltage alarm
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 768aed5..9a5bdb0 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
> This driver can also be built as a module. If so, the module
> will be called pwm-fan.
>
> +config SENSORS_RASPBERRYPI_HWMON
> + tristate "Raspberry Pi voltage monitor"
> + depends on RASPBERRYPI_FIRMWARE || COMPILE_TEST
> + help
> + If you say yes here you get support for voltage sensor on the
> + Raspberry Pi.
> +
> + This driver can also be built as a module. If so, the module
> + will be called raspberrypi-hwmon.
> +
> config SENSORS_SHT15
> tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
> depends on GPIOLIB || COMPILE_TEST
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index e7d52a3..a929770 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
> obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> +obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
> obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
> diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
> new file mode 100644
> index 0000000..6233e84
> --- /dev/null
> +++ b/drivers/hwmon/raspberrypi-hwmon.c
> @@ -0,0 +1,168 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Raspberry Pi voltage sensor driver
> + *
> + * Based on firmware/raspberrypi.c by Noralf Trønnes
> + *
> + * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
> + */
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/workqueue.h>
> +#include <soc/bcm2835/raspberrypi-firmware.h>
> +
> +#define UNDERVOLTAGE_STICKY_BIT BIT(16)
> +
> +struct rpi_hwmon_data {
> + struct device *hwmon_dev;
> + struct rpi_firmware *fw;
> + u32 last_throttled;
> + struct delayed_work get_values_poll_work;
> +};
> +
> +static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
> +{
> + u32 new_uv, old_uv, value;
> + int ret;
> +
> + /* Request firmware to clear sticky bits */
> + value = 0xffff;
> +
> + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> + &value, sizeof(value));
> + if (ret) {
> + dev_err_once(data->hwmon_dev, "Failed to get throttled (%d)\n",
> + ret);
> + return;
> + }
> +
> + new_uv = value & UNDERVOLTAGE_STICKY_BIT;
> + old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
> + data->last_throttled = value;
> +
> + if (new_uv == old_uv)
> + return;
> +
> + if (new_uv)
> + dev_crit(data->hwmon_dev, "Undervoltage detected!\n");
> + else
> + dev_info(data->hwmon_dev, "Voltage normalised\n");
> +
> + sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
> +}
> +
> +static void get_values_poll(struct work_struct *work)
> +{
> + struct rpi_hwmon_data *data;
> +
> + data = container_of(work, struct rpi_hwmon_data,
> + get_values_poll_work.work);
> +
> + rpi_firmware_get_throttled(data);
> +
> + /*
> + * We can't run faster than the sticky shift (100ms) since we get
> + * flipping in the sticky bits that are cleared.
> + */
> + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> +}
> +
> +static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct rpi_hwmon_data *data = dev_get_drvdata(dev);
> +
> + *val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
> + return 0;
> +}
> +
> +static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + return 0444;
> +}
> +
> +static const u32 rpi_in_config[] = {
> + HWMON_I_LCRIT_ALARM,
> + 0
> +};
> +
> +static const struct hwmon_channel_info rpi_in = {
> + .type = hwmon_in,
> + .config = rpi_in_config,
> +};
> +
> +static const struct hwmon_channel_info *rpi_info[] = {
> + &rpi_in,
> + NULL
> +};
> +
> +static const struct hwmon_ops rpi_hwmon_ops = {
> + .is_visible = rpi_is_visible,
> + .read = rpi_read,
> +};
> +
> +static const struct hwmon_chip_info rpi_chip_info = {
> + .ops = &rpi_hwmon_ops,
> + .info = rpi_info,
> +};
> +
> +static int rpi_hwmon_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct rpi_hwmon_data *data;
> + int ret;
> +
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->fw = platform_get_drvdata(to_platform_device(dev->parent));
> + if (!data->fw)
> + return -EPROBE_DEFER;
> +
I am a bit at loss here (and sorry I didn't bring this up before).
How would this ever be possible, given that the driver is registered
from the firmware driver ?
> + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> + &data->last_throttled,
> + sizeof(data->last_throttled));
> + if (ret)
> + return -ENODEV;
> +
> + data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
> + data,
> + &rpi_chip_info,
> + NULL);
> +
> + INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
> + platform_set_drvdata(pdev, data);
> +
> + if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
> + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> +
> + return PTR_ERR_OR_ZERO(data->hwmon_dev);
> +}
> +
> +static int rpi_hwmon_remove(struct platform_device *pdev)
> +{
> + struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
> +
> + cancel_delayed_work_sync(&data->get_values_poll_work);
> +
> + return 0;
> +}
> +
> +static struct platform_driver rpi_hwmon_driver = {
> + .probe = rpi_hwmon_probe,
> + .remove = rpi_hwmon_remove,
> + .driver = {
> + .name = "raspberrypi-hwmon",
> + },
> +};
> +module_platform_driver(rpi_hwmon_driver);
> +
> +MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
> +MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
> +MODULE_LICENSE("GPL v2");
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v8 04/15] clk: qcom: Add CPU clock driver for msm8996
From: kbuild test robot @ 2018-05-22 13:49 UTC (permalink / raw)
Cc: kbuild-all, mturquette, sboyd, robh, mark.rutland, viresh.kumar,
nm, lgirdwood, broonie, andy.gross, david.brown, catalin.marinas,
will.deacon, rjw, linux-clk, devicetree, linux-kernel, linux-pm,
linux-arm-msm, linux-soc, linux-arm-kernel, rnayak, ilialin,
amit.kucheria, nicolas.dechesne, celster, tfinkel
In-Reply-To: <1526555955-29960-5-git-send-email-ilialin@codeaurora.org>
[-- Attachment #1: Type: text/plain, Size: 1647 bytes --]
Hi Ilia,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.17-rc6]
[cannot apply to clk/clk-next next-20180517]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Ilia-Lin/CPU-scaling-support-for-msm8996/20180520-132401
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arm-allyesconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All errors (new ones prefixed by >>):
>> drivers/soc/qcom/kryo-l2-accessors.c:7:10: fatal error: asm/sysreg.h: No such file or directory
#include <asm/sysreg.h>
^~~~~~~~~~~~~~
compilation terminated.
vim +7 drivers/soc/qcom/kryo-l2-accessors.c
f4e14120 Ilia Lin 2018-05-17 @7 #include <asm/sysreg.h>
f4e14120 Ilia Lin 2018-05-17 8 #include <soc/qcom/kryo-l2-accessors.h>
f4e14120 Ilia Lin 2018-05-17 9
:::::: The code at line 7 was first introduced by commit
:::::: f4e14120d663ce6e4670516a66bc0158dc692c45 soc: qcom: Separate kryo l2 accessors from PMU driver
:::::: TO: Ilia Lin <ilialin@codeaurora.org>
:::::: CC: 0day robot <lkp@intel.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 64542 bytes --]
^ permalink raw reply
* Re: [PATCH RFC V2 2/6] hwmon: Add support for RPi voltage sensor
From: Stefan Wahren @ 2018-05-22 13:51 UTC (permalink / raw)
To: Rob Herring, Guenter Roeck, Eric Anholt, Mark Rutland,
Jonathan Corbet, Jean Delvare
Cc: linux-hwmon, devicetree, Florian Fainelli, Scott Branden,
linux-doc, Ray Jui, Phil Elwell, Noralf Trønnes,
bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel
In-Reply-To: <90a768aa-ee8c-1050-cf15-60637069dbdb@roeck-us.net>
Hi Guenter,
> Guenter Roeck <linux@roeck-us.net> hat am 22. Mai 2018 um 15:41 geschrieben:
>
>
> On 05/22/2018 04:21 AM, Stefan Wahren wrote:
> > Currently there is no easy way to detect undervoltage conditions on a
> > remote Raspberry Pi. This hwmon driver retrieves the state of the
> > undervoltage sensor via mailbox interface. The handling based on
> > Noralf's modifications to the downstream firmware driver. In case of
> > an undervoltage condition only an entry is written to the kernel log.
> >
> > CC: "Noralf Trønnes" <noralf@tronnes.org>
> > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> > ---
> > Documentation/hwmon/raspberrypi-hwmon | 22 +++++
> > drivers/hwmon/Kconfig | 10 ++
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/raspberrypi-hwmon.c | 168 ++++++++++++++++++++++++++++++++++
> > 4 files changed, 201 insertions(+)
> > create mode 100644 Documentation/hwmon/raspberrypi-hwmon
> > create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
> >
> > diff --git a/Documentation/hwmon/raspberrypi-hwmon b/Documentation/hwmon/raspberrypi-hwmon
> > new file mode 100644
> > index 0000000..3c92e2c
> > --- /dev/null
> > +++ b/Documentation/hwmon/raspberrypi-hwmon
> > @@ -0,0 +1,22 @@
> > +Kernel driver raspberrypi-hwmon
> > +===============================
> > +
> > +Supported boards:
> > + * Raspberry Pi A+ (via GPIO on SoC)
> > + * Raspberry Pi B+ (via GPIO on SoC)
> > + * Raspberry Pi 2 B (via GPIO on SoC)
> > + * Raspberry Pi 3 B (via GPIO on port expander)
> > + * Raspberry Pi 3 B+ (via PMIC)
> > +
> > +Author: Stefan Wahren <stefan.wahren@i2se.com>
> > +
> > +Description
> > +-----------
> > +
> > +This driver periodically polls a mailbox property of the VC4 firmware to detect
> > +undervoltage conditions.
> > +
> > +Sysfs entries
> > +-------------
> > +
> > +in0_lcrit_alarm Undervoltage alarm
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 768aed5..9a5bdb0 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
> > This driver can also be built as a module. If so, the module
> > will be called pwm-fan.
> >
> > +config SENSORS_RASPBERRYPI_HWMON
> > + tristate "Raspberry Pi voltage monitor"
> > + depends on RASPBERRYPI_FIRMWARE || COMPILE_TEST
> > + help
> > + If you say yes here you get support for voltage sensor on the
> > + Raspberry Pi.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called raspberrypi-hwmon.
> > +
> > config SENSORS_SHT15
> > tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
> > depends on GPIOLIB || COMPILE_TEST
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index e7d52a3..a929770 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> > obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> > obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
> > obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> > +obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
> > obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> > obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> > obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
> > diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
> > new file mode 100644
> > index 0000000..6233e84
> > --- /dev/null
> > +++ b/drivers/hwmon/raspberrypi-hwmon.c
> > @@ -0,0 +1,168 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Raspberry Pi voltage sensor driver
> > + *
> > + * Based on firmware/raspberrypi.c by Noralf Trønnes
> > + *
> > + * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
> > + */
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/workqueue.h>
> > +#include <soc/bcm2835/raspberrypi-firmware.h>
> > +
> > +#define UNDERVOLTAGE_STICKY_BIT BIT(16)
> > +
> > +struct rpi_hwmon_data {
> > + struct device *hwmon_dev;
> > + struct rpi_firmware *fw;
> > + u32 last_throttled;
> > + struct delayed_work get_values_poll_work;
> > +};
> > +
> > +static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
> > +{
> > + u32 new_uv, old_uv, value;
> > + int ret;
> > +
> > + /* Request firmware to clear sticky bits */
> > + value = 0xffff;
> > +
> > + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> > + &value, sizeof(value));
> > + if (ret) {
> > + dev_err_once(data->hwmon_dev, "Failed to get throttled (%d)\n",
> > + ret);
> > + return;
> > + }
> > +
> > + new_uv = value & UNDERVOLTAGE_STICKY_BIT;
> > + old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
> > + data->last_throttled = value;
> > +
> > + if (new_uv == old_uv)
> > + return;
> > +
> > + if (new_uv)
> > + dev_crit(data->hwmon_dev, "Undervoltage detected!\n");
> > + else
> > + dev_info(data->hwmon_dev, "Voltage normalised\n");
> > +
> > + sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
> > +}
> > +
> > +static void get_values_poll(struct work_struct *work)
> > +{
> > + struct rpi_hwmon_data *data;
> > +
> > + data = container_of(work, struct rpi_hwmon_data,
> > + get_values_poll_work.work);
> > +
> > + rpi_firmware_get_throttled(data);
> > +
> > + /*
> > + * We can't run faster than the sticky shift (100ms) since we get
> > + * flipping in the sticky bits that are cleared.
> > + */
> > + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> > +}
> > +
> > +static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
> > + u32 attr, int channel, long *val)
> > +{
> > + struct rpi_hwmon_data *data = dev_get_drvdata(dev);
> > +
> > + *val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
> > + return 0;
> > +}
> > +
> > +static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
> > + u32 attr, int channel)
> > +{
> > + return 0444;
> > +}
> > +
> > +static const u32 rpi_in_config[] = {
> > + HWMON_I_LCRIT_ALARM,
> > + 0
> > +};
> > +
> > +static const struct hwmon_channel_info rpi_in = {
> > + .type = hwmon_in,
> > + .config = rpi_in_config,
> > +};
> > +
> > +static const struct hwmon_channel_info *rpi_info[] = {
> > + &rpi_in,
> > + NULL
> > +};
> > +
> > +static const struct hwmon_ops rpi_hwmon_ops = {
> > + .is_visible = rpi_is_visible,
> > + .read = rpi_read,
> > +};
> > +
> > +static const struct hwmon_chip_info rpi_chip_info = {
> > + .ops = &rpi_hwmon_ops,
> > + .info = rpi_info,
> > +};
> > +
> > +static int rpi_hwmon_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct rpi_hwmon_data *data;
> > + int ret;
> > +
> > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + data->fw = platform_get_drvdata(to_platform_device(dev->parent));
> > + if (!data->fw)
> > + return -EPROBE_DEFER;
> > +
>
> I am a bit at loss here (and sorry I didn't bring this up before).
> How would this ever be possible, given that the driver is registered
> from the firmware driver ?
Do you refer to the (wrong) return code, the assumption that the parent must be a platform driver or a possible race?
>
> > + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> > + &data->last_throttled,
> > + sizeof(data->last_throttled));
> > + if (ret)
> > + return -ENODEV;
> > +
> > + data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
> > + data,
> > + &rpi_chip_info,
> > + NULL);
> > +
> > + INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
> > + platform_set_drvdata(pdev, data);
> > +
> > + if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
> > + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> > +
> > + return PTR_ERR_OR_ZERO(data->hwmon_dev);
> > +}
> > +
> > +static int rpi_hwmon_remove(struct platform_device *pdev)
> > +{
> > + struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
> > +
> > + cancel_delayed_work_sync(&data->get_values_poll_work);
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver rpi_hwmon_driver = {
> > + .probe = rpi_hwmon_probe,
> > + .remove = rpi_hwmon_remove,
> > + .driver = {
> > + .name = "raspberrypi-hwmon",
> > + },
> > +};
> > +module_platform_driver(rpi_hwmon_driver);
> > +
> > +MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
> > +MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
> > +MODULE_LICENSE("GPL v2");
> >
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [reset-control] How to initialize hardware state with the shared reset line?
From: Philipp Zabel @ 2018-05-22 13:56 UTC (permalink / raw)
To: Masahiro Yamada, Rob Herring, Lee Jones, Martin Blumenstingl
Cc: Hans de Goede, Felipe Balbi, DTML, Arnd Bergmann, Linus Walleij,
Linux Kernel Mailing List, linux-arm-kernel
In-Reply-To: <CAK7LNATZgJ4MxOFLUCNARWv3Zz=gpL-jGReDnBnArquiaXRWoQ@mail.gmail.com>
Hi Masahiro,
On Thu, 2018-05-10 at 18:16 +0900, Masahiro Yamada wrote:
> Hi.
>
>
> The previous thread was:
> "usb: dwc3: support clocks and resets for DWC3 core"
> https://patchwork.kernel.org/patch/10349623/
>
>
> I changed the subject because
> I think this is rather reset-control topic than USB.
Thank you for taking the time to write this up in such detail.
Indeed we were not expecting hardware that both defaults to deasserted
resets and does not have a power-on reset mechanism for the IP cores, so
yes, the framework currently assumes that all shared reset lines are
initially (or at some point in the past since power-on were) asserted.
I wonder if we are going to see this more often in the future, or
whether the Amlogic Meson SoCs will stay the exception.
> I am searching for a good way to initialize hardware devices
> in the following situation:
>
> - two or more hardware devices share the same reset line
So in general, at least during runtime, the driver must not expect reset
assertion to actually work, so the sharing device is not disturbed.
> - those devices are not reset before booting the kernel
> (i.e. flip flops in RTL are random state at driver probe)
>
> - the hardware IP is used by various SoCs,
> and this issue only appears only on some.
Unfortunately for configurable IP cores like the DW ones I am not sure
to what extent these can actually be considered identical when
implemented in different SoCs.
I would expect that the hardware IP's basic requirements, like whether
the reset must be asserted at some defined point during operation, or
whether it has to be a pulse of da specific duration, does not change
between implementations.
> Specifically, the DWC3 USB IP case is in my mind,
> but this should apply to any hardware in general.
>
>
>
>
> Issue in detail
> ---------------
>
> The DWC3 USB IP is very popular and used by various SoCs
> as you see in drivers/usb/dwc3/.
>
> SoCs are using the same RTL as provided by Synopsys.
> (SoC vendors could tweak the RTL if they like,
> but it would rarely happen because it would just be troublesome)
>
> So, let's assume we use the same IP with the same RTL version.
> It is *compatible* in terms of device tree.
> In this case, we should avoid differentiating the driver code per-SoC.
>
> In fact, we ended up with
> commit ff0a632 ("usb: dwc3: of-simple: add support for shared and
> pulsed reset lines")
> commit e8284db ("usb: dwc3: of-simple: add support for the Amlogic
> Meson GXL and AXG SoCs")
I'm surprised, didn't all Meson SoCs gain level reset support for this
purpose?
I agree this should not be needed in the drivers, and I'd be very happy
if we could make this work on Meson with the dwc3 driver always
requesting shared resets.
> This means, the difference that comes from the reset _provider_
> must be implemented in the reset _consumer_.
> This is unfortunate.
I am not entirely happy about the shared vs. exclusive naming of the
API, but I have no better names. By choosing between the two, the
driver states its requirements of the API, not whether the SoC it is
implemented in actually shares the reset line with other devices.
> So, I wonder if we can support it in sub-system level somehow
> instead of messing up the reset consumer driver.
I'd like to deflect and, hoping that this is an exception, ask whether
we could just do this in the driver?
> - The reset topology is SoC-dependent.
> A reset line is dedicated for single hardware for some SoCs,
> and shared by multiple hardware for others.
In which case the driver must be able to cope with shared reset lines,
so it should use the shared reset API.
> - The reset policy (pulse reset, level reset, or both of them)
> are SoC-dependent (reset controller dependent).
While that may be the case for some hardware IP, I don't think it is the
case for the dwc3 driver. It doesn't need to assert the reset at any
time during normal operation, and it works with level resets. The
pulsed/exclusive reset handling really is just a workaround for the
missing power-on reset.
> RTL generally contains state machines.
> The initial state of flip flops are undefined on power-on
> hence the initial state of state machines are also undefined.
>
> So, digital circuits needs explicit reset in general.
>
> In some cases, the reset is performed before booting the kernel.
>
> - power-on reset
> (FFs are put into defined state on power-on automatically)
>
> - a reset controller assert reset lines by default
> (FFs are fixed to defined state. If a reset consumer
> driver deasserts the reset line, HW starts from the define state)
>
> - Firmware may reset the hardware before booting the kernel
>
>
> If nothing above is done, and the reset line is shared by multiple devices,
> we do not have a good way to put the hardware into the sane state.
>
>
> Reset consumers choose the required reset type:
>
> - Shared reset:
> reset_control_assert / reset_control_deassert work like
> clock_disable / clock_enable.
> The consumer must be tolerant to the situation where the HW may
> not reset-asserted.
>
> - Exclusive reset:
> It is guaranteed reset_control_assert() really asserts the reset line,
> but only one reset consumer grabs the reset line at the same time.
Yes. The last part is a limitation of the current framework
implementation, but one that I'd like to keep as long as possible.
To support shared resets with guaranteed assertion, we'd need a
notification framework with a possibility for drivers to temporarily
suspend their own operations to allow a reset request from a different
device, or to veto reset requests if that is not possible. And drivers
that try to reset need to properly handle failed or deferred reset
requests. That would introduce a level of complexity that I'd very much
like to avoid.
> As above, the state machines in digital circuits must start from a
> defined state.
> So, we want exclusive reset to reset the FFs.
> However, this is too much requirement
> since it is a reset line is often shared.
For missing power-on resets, what we really want is not "exclusive
reset", and not even "guaranteed reset assertion". We want that after
the initial reset_deassert, the reset line is deasserted and the device
has been in reset *at any point in the past* since power-on.
> How to save such an Amlogic case?
I think this case can be solved by either just initializing the reset
lines to asserted in the reset controller driver (in which case the
controller driver needs to know about which lines have to be asserted,
and needs to support assert/deassert ops) or by adding a fallback from
.deassert to .reset for reset controllers that don't have level reset
hardware support. That is, if the first driver calls
reset_control_deassert, the reset controller driver actually issues a
reset pulse.
> Hogging solve the problem?
> --------------------------
>
>
> I was thinking of a solution.
>
> I may be missing something, but
> one solution might be reset hogging on the
> reset provider side. This allows us to describe
> the initial state of reset lines in the reset controller.
>
> The idea for "reset-hog" is similar to:
> - "gpio-hog" defined in
> Documentation/devicetree/bindings/gpio/gpio.txt
> - "assigned-clocks" defined in
> Documetation/devicetree/bindings/clock/clock-bindings.txt
>
>
>
> For example,
>
> reset-controller {
> ....
>
> line_a {
> reset-hog;
> resets = <1>;
> reset-assert;
> };
> }
>
>
> When the reset controller is registered,
> the reset ID '1' is asserted.
>
>
> So, all reset consumers that share the reset line '1'
> will start from the asserted state
> (i.e. defined state machine state).
>
>
> From the discussion with Martin Blumenstingl
> (https://lkml.org/lkml/2018/4/28/115),
> the problem for Amlogic is that
> the reset line is "de-asserted" by default.
> If so, the 'reset-hog' would fix the problem,
> and DWC3 driver would be able to use
> shared, level reset, I think.
>
>
> I think something may be missing from my mind,
> but I hope this will prompt the discussion,
> and we will find a better idea.
My question would is: do we need this complexity, or can this be solved
by just adding a workaround in the meson driver? If a lot of SoCs using
the simple reset driver had this requirement, I'd seriously consider
this, but it currently seems to me that this is just an issue with a
single SoC family.
regards
Philipp
^ permalink raw reply
* Re: [reset-control] How to initialize hardware state with the shared reset line?
From: Philipp Zabel @ 2018-05-22 14:04 UTC (permalink / raw)
To: Martin Blumenstingl, Masahiro Yamada
Cc: Rob Herring, Lee Jones, Hans de Goede, Felipe Balbi, DTML,
Arnd Bergmann, Linus Walleij, Linux Kernel Mailing List,
linux-arm-kernel
In-Reply-To: <CAFBinCBTTmPqD0UP8zzMBjsjzP9fJx9_pmhGz0_Vy-V+wmhLXQ@mail.gmail.com>
Hi Martin,
On Mon, 2018-05-21 at 12:40 +0200, Martin Blumenstingl wrote:
> Hello,
>
> On Mon, May 21, 2018 at 3:27 AM, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> > Hi.
> >
> >
> > 2018-05-20 19:57 GMT+09:00 Martin Blumenstingl
> > <martin.blumenstingl@googlemail.com>:
> > > Hi,
> > >
> > > On Thu, May 10, 2018 at 11:16 AM, Masahiro Yamada
> > > <yamada.masahiro@socionext.com> wrote:
> > > [snip]
> > > > I may be missing something, but
> > > > one solution might be reset hogging on the
> > > > reset provider side. This allows us to describe
> > > > the initial state of reset lines in the reset controller.
> > > >
> > > > The idea for "reset-hog" is similar to:
> > > > - "gpio-hog" defined in
> > > > Documentation/devicetree/bindings/gpio/gpio.txt
> > > > - "assigned-clocks" defined in
> > > > Documetation/devicetree/bindings/clock/clock-bindings.txt
> > > >
> > > >
> > > >
> > > > For example,
> > > >
> > > > reset-controller {
> > > > ....
> > > >
> > > > line_a {
> > > > reset-hog;
> > > > resets = <1>;
> > > > reset-assert;
> > > > };
> > > > }
> > > >
> > > >
> > > > When the reset controller is registered,
> > > > the reset ID '1' is asserted.
> > > >
> > > >
> > > > So, all reset consumers that share the reset line '1'
> > > > will start from the asserted state
> > > > (i.e. defined state machine state).
> > >
> > > I wonder if a "reset hog" can be board specific:
> > > - GPIO hogs are definitely board specific (meson-gxbb-odroidc2.dts for
> > > example uses it to take the USB hub out of reset)
> > > - assigned-clock-parents (and the like) can also be board specific (I
> > > made up a use-case since I don't know of any actual examples: board A
> > > uses an external XTAL while board B uses some other internal
> > > clock-source because it doesn't have an external XTAL)
> > >
> > > however, can reset lines be board specific? or in other words: do we
> > > need to describe them in device-tree?
> >
> > Indeed.
> >
> > I did not come up with board-specific cases.
> >
> > The problem we are discussing is SoC-specific,
> > and reset-controller drivers are definitely SoC-specific.
> >
> > So, I think the initial state can be coded in drivers instead of DT.
>
> OK, let's also hear Philipp's (reset framework maintainer) opinion on this
I'd like to know if there are other SoC families besides Amlogic Meson
that potentially could have this problem and about how many of the
resets that are documented in include/dt-bindings/reset/amlogic,meson*
we are actually talking. Are all of those initially deasserted and none
of the connected peripherals have power-on reset mechanisms?
> > > we could extend struct reset_controller_dev (= reset controller
> > > driver) if they are not board specific:
> > > - either assert all reset lines by default except if they are listed
> > > in a new field (may break backwards compatibility, requires testing of
> > > all reset controller drivers)
> >
> > This is quite simple, but I am afraid there are some cases where the forcible
> > reset-assert is not preferred.
> >
> > For example, the earlycon. When we use earlycon, we would expect it has been
> > initialized by a boot-loader, or something.
> > If it is reset-asserted on the while, the console output
> > will not be good.
>
> indeed, so let's skip this idea
Maybe we should at first add initial reset assertion to the Meson driver
on a case by case bases?
We can't add required reset hog DT bindings to the Meson reset
controller anyway without breaking DT backwards compatibility.
> > > - specify a list of reset lines and their desired state (or to keep it
> > > easy: specify a list of reset lines that should be asserted)
> > > (I must admit that this is basically your idea but the definition is
> > > moved from device-tree to the reset controller driver)
> >
> > Yes, I think the list of "reset line ID" and "init state" pairs
> > would be nicer.
>
> $ grep -R "of_reset_n_cells = [^1]" drivers/reset/
> drivers/reset/reset-berlin.c: priv->rcdev.of_reset_n_cells = 2;
> drivers/reset/hisilicon/reset-hi3660.c: rc->rst.of_reset_n_cells = 2;
> drivers/reset/reset-ti-sci.c: data->rcdev.of_reset_n_cells = 2;
> drivers/reset/reset-lantiq.c: priv->rcdev.of_reset_n_cells = 2;
>
> everything else uses only one reset cell
> from the lantiq reset dt-binding documentation: "The first cell takes
> the reset set bit and the second cell takes the status bit."
>
> I'm not sure what to do with drivers that specify != 1 reset-cell
> though if we use a simple "init state pair"
> maybe Philipp can share his opinion on this one as well
See above, so far I am not convinced (either way) whether this should be
described in the DT at all.
> > > any "chip" specific differences could be expressed by using a
> > > different of_device_id
> > >
> > > one the other hand: your "reset hog" solution looks fine to me if
> > > reset lines can be board specific
> > >
> > > > From the discussion with Martin Blumenstingl
> > > > (https://lkml.org/lkml/2018/4/28/115),
> > > > the problem for Amlogic is that
> > > > the reset line is "de-asserted" by default.
> > > > If so, the 'reset-hog' would fix the problem,
> > > > and DWC3 driver would be able to use
> > > > shared, level reset, I think.
> > >
> > > I think you are right: if we could control the initial state then we
> > > should be able to use level resets
> >
> >
> > Even further, can we drop the shared reset_control_reset() support, maybe?
> > (in other words, revert commit 7da33a37b48f11)
>
> I believe we need to keep this if there's hardware out there:
> - where the reset controller only supports reset pulses
> - at least one reset line is shared between multiple devices
>
> I didn't have a closer look at the Amlogic Meson6 SoC yet, but I think
> it matches above criteria. as far as I know:
> - the USB situation there is similar to Meson8b (USB controllers and
> PHYs share a reset line)
> - it uses an older reset controller IP block which does not support
> level resets (only reset pulses)
See my answer to Masahiro's first mail. I think somebody suggested in
the past to add a fallback from the deassert to the reset op. I think
this is something that should work in this case.
regards
Philipp
^ permalink raw reply
* Re: [PATCH v2 03/14] dt-bindings: qcom_nandc: make nand-ecc-strength optional
From: Abhishek Sahu @ 2018-05-22 14:08 UTC (permalink / raw)
To: Miquel Raynal
Cc: Boris Brezillon, David Woodhouse, Brian Norris, Marek Vasut,
Richard Weinberger, linux-arm-msm, linux-kernel, linux-mtd,
Andy Gross, Archit Taneja, Rob Herring, Mark Rutland, devicetree
In-Reply-To: <20180521163254.71ade105@xps13>
On 2018-05-21 20:02, Miquel Raynal wrote:
> Hi Abhishek,
>
> On Thu, 3 May 2018 17:50:30 +0530, Abhishek Sahu
> <absahu@codeaurora.org> wrote:
>
>> Now, nand-ecc-strength is optional. If specified in DT, then
>> controller will use this ECC strength otherwise ECC strength will
>> be calculated according to chip requirement and available OOB size.
>
> Same comment as before: don't start the commit log with "now,".
>
> Thanks,
> Miquèl
Thanks Miquel.
I will change the commit message.
Regards,
Abhishek
^ 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