* Re: [V8, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings
From: Tomasz Figa @ 2020-05-21 18:59 UTC (permalink / raw)
To: Dongchun Zhu, Rob Herring
Cc: Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab,
Andy Shevchenko, Mark Rutland, Sakari Ailus, Nicolas Boichat,
Matthias Brugger, Cao Bing Bu, srv_heupstream,
moderated list:ARM/Mediatek SoC support,
list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>,,
Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo,
Shengnan Wang (王圣男)
In-Reply-To: <1589251221.8804.289.camel@mhfsdcap03>
Hi Dongchun, Rob,
On Tue, May 12, 2020 at 4:41 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
>
> Hi Rob,
>
> Thanks for the review.
>
> On Mon, 2020-05-11 at 11:02 -0500, Rob Herring wrote:
> > On Sat, May 09, 2020 at 04:06:26PM +0800, Dongchun Zhu wrote:
[snip]
> > > + port {
> > > + wcam_out: endpoint {
> > > + remote-endpoint = <&mipi_in_wcam>;
> > > + data-lanes = <1>;
> >
> > This doesn't match the schema which says this should be 4 entries.
> >
>
> Property "data-lanes" shows the number of lanes that sensor supports.
> If this property is omitted, four-lane operation is assumed.
> For OV02A10, it is one-lane operation, so the property is supposed to be
> set to <1>.
>
To clarify on this, the ov02a10 sensor supports only 1 lane. It's not
a driver limitation.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH v2 0/4] TI K3 DSP remoteproc driver for C66x DSPs
From: Bjorn Andersson @ 2020-05-21 19:01 UTC (permalink / raw)
To: Suman Anna
Cc: Rob Herring, Mathieu Poirier, Lokesh Vutla, linux-remoteproc,
devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <5f84465e-8f63-51b4-4758-59c85d3ad597@ti.com>
On Thu 21 May 11:59 PDT 2020, Suman Anna wrote:
> Hi Bjorn,
>
> On 5/20/20 7:10 PM, Suman Anna wrote:
> > Hi All,
> >
> > The following is v2 of the K3 DSP remoteproc driver supporting the C66x DSPs
> > on the TI K3 J721E SoCs. The patches are based on the latest commit on the
> > rproc-next branch, 7dcef3988eed ("remoteproc: Fix an error code in
> > devm_rproc_alloc()").
>
> I realized I also had the R5F patches on my branch, so the third patch won't
> apply cleanly (conflict on Makefile). Let me know if you want a new revision
> posted for you to pick up the series.
>
That should be fine, thanks for the heads up!
Will give Mathieu a day or two to take a look as well.
Regards,
Bjorn
> regards
> Suman
>
> >
> > v2 includes a new remoteproc core patch (patch 1) that adds an OF helper
> > for parsing the firmware-name property. This is refactored out to avoid
> > replicating the code in various remoteproc drivers. Please see the
> > individual patches for detailed changes.
> >
> > The main dependent patches from the previous series are now staged in
> > rproc-next branch. The only dependency for this series is the common
> > ti-sci-proc helper between R5 and DSP drivers [1]. Please see the initial
> > cover-letter [2] for v1 details.
> >
> > regards
> > Suman
> >
> > [1] https://patchwork.kernel.org/patch/11456379/
> > [2] https://patchwork.kernel.org/cover/11458573/
> >
> > Suman Anna (4):
> > remoteproc: Introduce rproc_of_parse_firmware() helper
> > dt-bindings: remoteproc: Add bindings for C66x DSPs on TI K3 SoCs
> > remoteproc/k3-dsp: Add a remoteproc driver of K3 C66x DSPs
> > remoteproc/k3-dsp: Add support for L2RAM loading on C66x DSPs
> >
> > .../bindings/remoteproc/ti,k3-dsp-rproc.yaml | 190 +++++
> > drivers/remoteproc/Kconfig | 13 +
> > drivers/remoteproc/Makefile | 1 +
> > drivers/remoteproc/remoteproc_core.c | 23 +
> > drivers/remoteproc/remoteproc_internal.h | 2 +
> > drivers/remoteproc/ti_k3_dsp_remoteproc.c | 773 ++++++++++++++++++
> > 6 files changed, 1002 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/remoteproc/ti,k3-dsp-rproc.yaml
> > create mode 100644 drivers/remoteproc/ti_k3_dsp_remoteproc.c
> >
>
^ permalink raw reply
* Re: [PATCH 2/4] remoteproc: introduce version element into resource type field
From: Suman Anna @ 2020-05-21 19:06 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Rob Herring, Mathieu Poirier, Clement Leger, Loic Pallardy,
Arnaud Pouliquen, Lokesh Vutla, linux-remoteproc, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <20200521175421.GI408178@builder.lan>
Hi Bjorn,
On 5/21/20 12:54 PM, Bjorn Andersson wrote:
> On Wed 25 Mar 13:46 PDT 2020, Suman Anna wrote:
>
>> The current remoteproc core has supported only 32-bit remote
>> processors and as such some of the current resource structures
>> may not scale well for 64-bit remote processors, and would
>> require new versions of resource types. Each resource is currently
>> identified by a 32-bit type field. Introduce the concept of version
>> for these resource types by overloading this 32-bit type field
>> into two 16-bit version and type fields with the existing resources
>> behaving as version 0 thereby providing backward compatibility.
>>
>> The version field is passed as an additional argument to each of
>> the handler functions, and all the existing handlers are updated
>> accordingly. Each specific handler will be updated on a need basis
>> when a new version of the resource type is added.
>>
>
> I really would prefer that we add additional types for the new
> structures, neither side will be compatible with new versions without
> enhancements to their respective implementations anyways.
OK.
>
>> An alternate way would be to introduce the new types as completely
>> new resource types which would require additional customization of
>> the resource handlers based on the 32-bit or 64-bit mode of a remote
>> processor, and introduction of an additional mode flag to the rproc
>> structure.
>>
>
> What would this "mode" indicate? If it's version 0 or 1?
No, for indicating if the remoteproc is 32-bit or 64-bit and adjust the
loading handlers if the resource types need to be segregated accordingly.
>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> ---
>> drivers/remoteproc/remoteproc_core.c | 25 +++++++++++++++----------
>> drivers/remoteproc/remoteproc_debugfs.c | 17 ++++++++++-------
>> include/linux/remoteproc.h | 8 +++++++-
>> 3 files changed, 32 insertions(+), 18 deletions(-)
>>
> [..]
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 77788a4bb94e..526d3cb45e37 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -86,7 +86,13 @@ struct resource_table {
>> * this header, and it should be parsed according to the resource type.
>> */
>> struct fw_rsc_hdr {
>> - u32 type;
>> + union {
>> + u32 type;
>> + struct {
>> + u16 t;
>> + u16 v;
>> + } st;
>
> I see your "type" is little endian...
Yeah, definitely a draw-back if we want to support big-endian rprocs. Do
you have any remoteprocs following big-endian? All TI remoteprocs are
little-endian except for really old ones.
regards
Suman
^ permalink raw reply
* Re: [PATCH V2 2/3] mmc: sdhci-msm: Use internal voltage control
From: Bjorn Andersson @ 2020-05-21 19:07 UTC (permalink / raw)
To: Veerabhadrarao Badiganti
Cc: adrian.hunter, ulf.hansson, robh+dt, linux-mmc, linux-kernel,
linux-arm-msm, devicetree, Asutosh Das, Vijay Viswanath,
Andy Gross
In-Reply-To: <1590074615-10787-3-git-send-email-vbadigan@codeaurora.org>
On Thu 21 May 08:23 PDT 2020, Veerabhadrarao Badiganti wrote:
> On qcom SD host controllers voltage switching be done after the HW
> is ready for it. The HW informs its readiness through power irq.
> The voltage switching should happen only then.
>
> Use the internal voltage switching and then control the voltage
> switching using power irq.
>
> Set the regulator load as well so that regulator can be configured
> in LPM mode when in is not being used.
>
> Co-developed-by: Asutosh Das <asutoshd@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> Co-developed-by: Vijay Viswanath <vviswana@codeaurora.org>
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> Co-developed-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
Looks better, thanks.
> ---
> drivers/mmc/host/sdhci-msm.c | 207 +++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 198 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
[..]
> static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
> @@ -1298,6 +1302,71 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
> sdhci_msm_hs400(host, &mmc->ios);
> }
>
> +static int sdhci_msm_set_vmmc(struct mmc_host *mmc)
> +{
> + int ret;
> +
> + if (IS_ERR(mmc->supply.vmmc))
> + return 0;
> +
> + ret = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, mmc->ios.vdd);
> + if (ret)
> + dev_err(mmc_dev(mmc), "%s: vmmc set ocr with vdd=%d failed: %d\n",
> + mmc_hostname(mmc), mmc->ios.vdd, ret);
Missed this one on v1, in the event that mmc_regulator_set_ocr() return
a non-zero value it has already printed an error message. So please
replace the tail with just:
return mmc_regulator_set_ocr(...);
> +
> + return ret;
> +}
> +
> +static int sdhci_msm_set_vqmmc(struct sdhci_msm_host *msm_host,
> + struct mmc_host *mmc, bool level)
> +{
> + int load, ret;
> + struct mmc_ios ios;
> +
> + if (IS_ERR(mmc->supply.vqmmc) ||
> + (mmc->ios.power_mode == MMC_POWER_UNDEFINED) ||
> + (msm_host->vqmmc_enabled == level))
> + return 0;
> +
> + if (msm_host->vqmmc_load) {
> + load = level ? msm_host->vqmmc_load : 0;
> + ret = regulator_set_load(mmc->supply.vqmmc, load);
Sorry for the late reply on v1, but please see my explanation regarding
load and always-on regulators there.
> + if (ret) {
> + dev_err(mmc_dev(mmc), "%s: vqmmc set load failed: %d\n",
> + mmc_hostname(mmc), ret);
> + goto out;
> + }
> + }
> +
> + if (level) {
> + /* Set the IO voltage regulator to default voltage level */
> + if (msm_host->caps_0 & CORE_3_0V_SUPPORT)
> + ios.signal_voltage = MMC_SIGNAL_VOLTAGE_330;
> + else if (msm_host->caps_0 & CORE_1_8V_SUPPORT)
> + ios.signal_voltage = MMC_SIGNAL_VOLTAGE_180;
> +
> + if (msm_host->caps_0 & CORE_VOLT_SUPPORT) {
> + ret = mmc_regulator_set_vqmmc(mmc, &ios);
> + if (ret < 0) {
> + dev_err(mmc_dev(mmc), "%s: vqmmc set volgate failed: %d\n",
> + mmc_hostname(mmc), ret);
> + goto out;
> + }
> + }
> + ret = regulator_enable(mmc->supply.vqmmc);
> + } else {
> + ret = regulator_disable(mmc->supply.vqmmc);
> + }
> +
> + if (ret)
> + dev_err(mmc_dev(mmc), "%s: vqmm %sable failed: %d\n",
> + mmc_hostname(mmc), level ? "en":"dis", ret);
> + else
> + msm_host->vqmmc_enabled = level;
> +out:
> + return ret;
> +}
[..]
> +static int sdhci_msm_start_signal_voltage_switch(struct mmc_host *mmc,
> + struct mmc_ios *ios)
> +{
> + struct sdhci_host *host = mmc_priv(mmc);
> + u16 ctrl, status;
> +
> + /*
> + * Signal Voltage Switching is only applicable for Host Controllers
> + * v3.00 and above.
> + */
> + if (host->version < SDHCI_SPEC_300)
> + return 0;
> +
> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> +
> + switch (ios->signal_voltage) {
> + case MMC_SIGNAL_VOLTAGE_330:
> + if (!(host->flags & SDHCI_SIGNALING_330))
> + return -EINVAL;
> +
> + /* Set 1.8V Signal Enable in the Host Control2 register to 0 */
> + ctrl &= ~SDHCI_CTRL_VDD_180;
> + break;
> + case MMC_SIGNAL_VOLTAGE_180:
> + if (!(host->flags & SDHCI_SIGNALING_180))
> + return -EINVAL;
> +
> + /*
> + * Enable 1.8V Signal Enable in the Host Control2
> + * register
> + */
> + ctrl |= SDHCI_CTRL_VDD_180;
> + break;
> + case MMC_SIGNAL_VOLTAGE_120:
> + if (!(host->flags & SDHCI_SIGNALING_120))
> + return -EINVAL;
> + return 0;
> + default:
> + /* No signal voltage switch required */
> + return 0;
> + }
> +
> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
> +
> + /* Wait for 5ms */
> + usleep_range(5000, 5500);
> +
> + /* regulator output should be stable within 5 ms */
> + status = !!(ctrl & SDHCI_CTRL_VDD_180);
> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> + if (!!(ctrl & SDHCI_CTRL_VDD_180) == status)
You should be able to drop the !! both here and when assigning status.
Overall this looks neater, thanks for reworking it.
Regards,
Bjorn
^ permalink raw reply
* Re: [PATCH 2/4] remoteproc: introduce version element into resource type field
From: Bjorn Andersson @ 2020-05-21 19:21 UTC (permalink / raw)
To: Suman Anna
Cc: Rob Herring, Mathieu Poirier, Clement Leger, Loic Pallardy,
Arnaud Pouliquen, Lokesh Vutla, linux-remoteproc, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <b338480e-c586-f988-f5b6-784551b7beb6@ti.com>
On Thu 21 May 12:06 PDT 2020, Suman Anna wrote:
> Hi Bjorn,
>
> On 5/21/20 12:54 PM, Bjorn Andersson wrote:
> > On Wed 25 Mar 13:46 PDT 2020, Suman Anna wrote:
> >
> > > The current remoteproc core has supported only 32-bit remote
> > > processors and as such some of the current resource structures
> > > may not scale well for 64-bit remote processors, and would
> > > require new versions of resource types. Each resource is currently
> > > identified by a 32-bit type field. Introduce the concept of version
> > > for these resource types by overloading this 32-bit type field
> > > into two 16-bit version and type fields with the existing resources
> > > behaving as version 0 thereby providing backward compatibility.
> > >
> > > The version field is passed as an additional argument to each of
> > > the handler functions, and all the existing handlers are updated
> > > accordingly. Each specific handler will be updated on a need basis
> > > when a new version of the resource type is added.
> > >
> >
> > I really would prefer that we add additional types for the new
> > structures, neither side will be compatible with new versions without
> > enhancements to their respective implementations anyways.
>
> OK.
>
> >
> > > An alternate way would be to introduce the new types as completely
> > > new resource types which would require additional customization of
> > > the resource handlers based on the 32-bit or 64-bit mode of a remote
> > > processor, and introduction of an additional mode flag to the rproc
> > > structure.
> > >
> >
> > What would this "mode" indicate? If it's version 0 or 1?
>
> No, for indicating if the remoteproc is 32-bit or 64-bit and adjust the
> loading handlers if the resource types need to be segregated accordingly.
>
Sorry, I think I'm misunderstanding something. Wouldn't your 64-bit
remote processor need different firmware from your 32-bit processor
anyways, if you want to support the wider resource? And you would pack
your firmware with the appropriate resource types?
Afaict the bit width of your remote processor, busses or memory is
unrelated to the choice of number of bits used to express things in the
resource table.
Regards,
Bjorn
^ permalink raw reply
* Re: [PATCH v2] arm: dts: am33xx-bone-common: add gpio-line-names
From: Drew Fustini @ 2020-05-21 19:23 UTC (permalink / raw)
To: Grygorii Strashko
Cc: Robert Nelson, Linus Walleij, Benoît Cousson, Tony Lindgren,
Rob Herring, Linux-OMAP, devicetree, linux kernel, Jason Kridner
In-Reply-To: <cdf82f9c-48fb-49a0-99e4-926dc292c109@ti.com>
On Thu, May 21, 2020 at 08:26:42PM +0300, Grygorii Strashko wrote:
>
>
> On 21/05/2020 17:41, Robert Nelson wrote:
> > > Not sure if it should be in am335x-bone-common.dtsi.
> > >
> > > For example:
> > > am335x-boneblack.dts
> > > #include "am335x-bone-common.dtsi"
> > > #include "am335x-boneblack-common.dtsi" <-- hdmi defined only here
> >
> > Ah crap, yeah that's a good point.. So if we stick it in...
> > am335x-boneblack-common.dtsi
> >
> > Then the Black-Wireless now has Ethernet...
> >
> > am335x-boneblack-wireless.dts
> > #include "am335x-bone-common.dtsi"
> > #include "am335x-boneblack-common.dtsi"
> >
> > It's going to be ugly, copy and paste mess, but i guess we might as
> > well stick it in the device " am335x-boneblack.dts"?
>
> Seems like.
>
> Unfortunately, there is no way to partially overwrite "gpio-line-names"
> property.
>
> --
> Best regards,
> grygorii
Thank you for pointing out that those gpio-line-names are really
specific to the BeagleBone Black and not applicable for other boards
like the BeagleBone Green which lack eMMC.
I will post a new patch for am335x-boneblack.dts.
After that is reviewed and applied, I will then create patches for the
the other BeagleBone models.
thanks,
drew
^ permalink raw reply
* Re: [PATCH 2/4] remoteproc: introduce version element into resource type field
From: Suman Anna @ 2020-05-21 19:29 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Rob Herring, Mathieu Poirier, Clement Leger, Loic Pallardy,
Arnaud Pouliquen, Lokesh Vutla, linux-remoteproc, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <20200521192146.GO408178@builder.lan>
On 5/21/20 2:21 PM, Bjorn Andersson wrote:
> On Thu 21 May 12:06 PDT 2020, Suman Anna wrote:
>
>> Hi Bjorn,
>>
>> On 5/21/20 12:54 PM, Bjorn Andersson wrote:
>>> On Wed 25 Mar 13:46 PDT 2020, Suman Anna wrote:
>>>
>>>> The current remoteproc core has supported only 32-bit remote
>>>> processors and as such some of the current resource structures
>>>> may not scale well for 64-bit remote processors, and would
>>>> require new versions of resource types. Each resource is currently
>>>> identified by a 32-bit type field. Introduce the concept of version
>>>> for these resource types by overloading this 32-bit type field
>>>> into two 16-bit version and type fields with the existing resources
>>>> behaving as version 0 thereby providing backward compatibility.
>>>>
>>>> The version field is passed as an additional argument to each of
>>>> the handler functions, and all the existing handlers are updated
>>>> accordingly. Each specific handler will be updated on a need basis
>>>> when a new version of the resource type is added.
>>>>
>>>
>>> I really would prefer that we add additional types for the new
>>> structures, neither side will be compatible with new versions without
>>> enhancements to their respective implementations anyways.
>>
>> OK.
>>
>>>
>>>> An alternate way would be to introduce the new types as completely
>>>> new resource types which would require additional customization of
>>>> the resource handlers based on the 32-bit or 64-bit mode of a remote
>>>> processor, and introduction of an additional mode flag to the rproc
>>>> structure.
>>>>
>>>
>>> What would this "mode" indicate? If it's version 0 or 1?
>>
>> No, for indicating if the remoteproc is 32-bit or 64-bit and adjust the
>> loading handlers if the resource types need to be segregated accordingly.
>>
>
> Sorry, I think I'm misunderstanding something. Wouldn't your 64-bit
> remote processor need different firmware from your 32-bit processor
> anyways, if you want to support the wider resource? And you would pack
> your firmware with the appropriate resource types?
Yes, that's correct.
>
> Afaict the bit width of your remote processor, busses or memory is
> unrelated to the choice of number of bits used to express things in the
> resource table.
I would have to add the new resource type to the loading_handlers right,
so it is a question of whether we want to impose any restrictions in
remoteproc core or not from supporting a certain resource type (eg: I
don't expect RSC_TRACE entries on 64-bit processors).
regards
Suman
^ permalink raw reply
* Re: [V8, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver
From: Tomasz Figa @ 2020-05-21 19:32 UTC (permalink / raw)
To: Dongchun Zhu
Cc: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt,
mark.rutland, sakari.ailus, drinkcat, matthias.bgg, bingbu.cao,
srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang,
linux-media, devicetree, louis.kuo, shengnan.wang
In-Reply-To: <20200509080627.23222-3-dongchun.zhu@mediatek.com>
Hi Dongchun,
On Sat, May 09, 2020 at 04:06:27PM +0800, Dongchun Zhu wrote:
> Add a V4L2 sub-device driver for OV02A10 image sensor.
>
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> ---
> MAINTAINERS | 1 +
> drivers/media/i2c/Kconfig | 13 +
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/ov02a10.c | 1094 +++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 1109 insertions(+)
> create mode 100644 drivers/media/i2c/ov02a10.c
>
Thank you for the patch. Please see my comments inline.
[snip]
> +struct ov02a10 {
> + u32 eclk_freq;
> + u32 mipi_clock_tx_speed;
> +
> + struct clk *eclk;
> + struct gpio_desc *pd_gpio;
> + struct gpio_desc *n_rst_gpio;
> + struct regulator_bulk_data supplies[OV02A10_NUM_SUPPLIES];
> +
> + bool streaming;
> + bool upside_down;
> + bool mipi_clock_tx_speed_select_enable;
> + bool mipi_clock_hs_mode_enable;
> +
> + /*
> + * Serialize control access, get/set format, get selection
> + * and start streaming.
> + */
> + struct mutex mutex;
> + struct v4l2_subdev subdev;
> + struct media_pad pad;
> + struct v4l2_ctrl *anal_gain;
> + struct v4l2_ctrl *exposure;
> + struct v4l2_ctrl *hblank;
> + struct v4l2_ctrl *vblank;
> + struct v4l2_ctrl *test_pattern;
> + struct v4l2_mbus_framefmt fmt;
nit: Remove the tabs between types and names and use single spaces. As you
can see above, even tabs don't guarantee equal alignment. And they actually
make adding fields more difficult, because if a longer field is added, the
alignment breaks.
[snip]
> +static int ov02a10_read_smbus(struct ov02a10 *ov02a10, unsigned char reg,
> + unsigned char *val)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + int ret;
> +
> + ret = i2c_smbus_read_byte_data(client, reg);
> +
nit: Unnecessary blank line.
> + if (ret < 0)
> + return ret;
> +
> + *val = (unsigned char)ret;
> +
> + return 0;
> +}
[snip]
> +static int __maybe_unused ov02a10_power_on(struct device *dev)
How is it possible that this function may be unused? If this driver allows
disabling runtime PM, then there is still a need to configure the clock,
regulator and GPIO. If not, there shouldn't be a case where this function
is unused.
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> + int ret;
> +
> + gpiod_set_value_cansleep(ov02a10->n_rst_gpio, 0);
> + gpiod_set_value_cansleep(ov02a10->pd_gpio, 0);
> +
> + ret = clk_prepare_enable(ov02a10->eclk);
> + if (ret < 0) {
> + dev_err(dev, "failed to enable eclk\n");
> + return ret;
> + }
> +
> + ret = regulator_bulk_enable(OV02A10_NUM_SUPPLIES, ov02a10->supplies);
> + if (ret < 0) {
> + dev_err(dev, "failed to enable regulators\n");
> + goto disable_clk;
> + }
> + usleep_range(5000, 6000);
> +
> + gpiod_set_value_cansleep(ov02a10->pd_gpio, 1);
This is a "powerdown" GPIO. It must be set to 0 if the sensor is to be
powered on.
> + usleep_range(5000, 6000);
> +
> + gpiod_set_value_cansleep(ov02a10->n_rst_gpio, 1);
> + usleep_range(5000, 6000);
> +
> + ret = ov02a10_check_sensor_id(ov02a10);
> + if (ret)
> + goto disable_regulator;
> +
> + return 0;
> +
> +disable_regulator:
> + regulator_bulk_disable(OV02A10_NUM_SUPPLIES, ov02a10->supplies);
> +disable_clk:
> + clk_disable_unprepare(ov02a10->eclk);
> +
> + return ret;
> +}
> +
> +static int __maybe_unused ov02a10_power_off(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> +
> + gpiod_set_value_cansleep(ov02a10->n_rst_gpio, 0);
> + clk_disable_unprepare(ov02a10->eclk);
> + gpiod_set_value_cansleep(ov02a10->pd_gpio, 0);
Similar comment as above. To power off the sensor, the "powerdown" GPIO
needs to be active, i.e. 1.
[snip]
> +/*
> + * ov02a10_set_exposure - Function called when setting exposure time
> + * @priv: Pointer to device structure
> + * @val: Variable for exposure time, in the unit of micro-second
> + *
> + * Set exposure time based on input value.
> + *
> + * Return: 0 on success
> + */
> +static int ov02a10_set_exposure(struct ov02a10 *ov02a10, int val)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + int ret;
> +
> + ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> + if (ret < 0)
> + return ret;
How does this page switch work? According to the documentation I have, the
register allows selecting between a few different pages. However, there
should be two page pointers - one for the AP and the other for the sensor,
so that when the AP is programming page X, the sensor can have consistent
settings from page Y. But here we only set one register and always with
page 1.
> +
> + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_EXPOSURE_H,
> + val >> OV02A10_EXP_SHIFT);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_EXPOSURE_L, val);
> + if (ret < 0)
> + return ret;
> +
> + return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> + REG_ENABLE);
This patch defines REG_GLOBAL_EFFECTIVE to 0x01. I don't see such register
mentioned in the documentation.
> +}
> +
> +static int ov02a10_set_gain(struct ov02a10 *ov02a10, int val)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + int ret;
> +
> + ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_GAIN, val);
> + if (ret < 0)
> + return ret;
> +
> + return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> + REG_ENABLE);
> +}
> +
> +static int ov02a10_set_vblank(struct ov02a10 *ov02a10, int val)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + u32 vts = val + ov02a10->cur_mode->height - OV02A10_BASIC_LINE;
> + int ret;
> +
> + ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_H,
> + vts >> OV02A10_VTS_SHIFT);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_L, vts);
> + if (ret < 0)
> + return ret;
> +
> + return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> + REG_ENABLE);
> +}
> +
> +static int ov02a10_set_test_pattern(struct ov02a10 *ov02a10, int pattern)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + int ret;
> +
> + if (pattern)
> + pattern = OV02A10_TEST_PATTERN_ENABLE;
Is this necessary? Our control can be 0 for disabled and 1 for color bars.
The latter is the same as the above macro.
[snip]
> +static int ov02a10_initialize_controls(struct ov02a10 *ov02a10)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + const struct ov02a10_mode *mode;
> + struct v4l2_ctrl_handler *handler;
> + struct v4l2_ctrl *ctrl;
> + u64 exposure_max;
> + u32 pixel_rate, h_blank;
> + int ret;
> +
> + handler = &ov02a10->ctrl_handler;
> + mode = ov02a10->cur_mode;
> + ret = v4l2_ctrl_handler_init(handler, 7);
> + if (ret)
> + return ret;
> +
> + handler->lock = &ov02a10->mutex;
> +
> + ctrl = v4l2_ctrl_new_int_menu(handler, NULL, V4L2_CID_LINK_FREQ, 0, 0,
> + link_freq_menu_items);
> + if (ctrl)
> + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> + pixel_rate = to_pixel_rate(0);
> + v4l2_ctrl_new_std(handler, NULL, V4L2_CID_PIXEL_RATE, 0, pixel_rate, 1,
> + pixel_rate);
> +
> + h_blank = mode->hts_def - mode->width;
> + ov02a10->hblank = v4l2_ctrl_new_std(handler, NULL, V4L2_CID_HBLANK,
> + h_blank, h_blank, 1, h_blank);
> + if (ov02a10->hblank)
> + ov02a10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
Do we need to hold a pointer to this control? We don't seem to ever access
it anywhere else in the driver.
> + ov02a10->vblank = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> + V4L2_CID_VBLANK, mode->vts_def -
> + mode->height,
> + OV02A10_VTS_MAX - mode->height, 1,
> + mode->vts_def - mode->height);
> +
Ditto.
> + exposure_max = mode->vts_def - 4;
> + ov02a10->exposure = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> + V4L2_CID_EXPOSURE,
> + OV02A10_EXPOSURE_MIN,
> + exposure_max,
> + OV02A10_EXPOSURE_STEP,
> + mode->exp_def);
> +
> + ov02a10->anal_gain = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> + V4L2_CID_ANALOGUE_GAIN,
> + OV02A10_GAIN_MIN,
> + OV02A10_GAIN_MAX,
> + OV02A10_GAIN_STEP,
> + OV02A10_GAIN_DEFAULT);
Ditto.
> +
> + ov02a10->test_pattern =
> + v4l2_ctrl_new_std_menu_items(handler, &ov02a10_ctrl_ops,
> + V4L2_CID_TEST_PATTERN,
> + ARRAY_SIZE(ov02a10_test_pattern_menu) -
> + 1, 0, 0,
> + ov02a10_test_pattern_menu);
Ditto.
[snip]
> + ov02a10->pd_gpio = devm_gpiod_get(dev, "powerdown", GPIOD_OUT_HIGH);
> + if (IS_ERR(ov02a10->pd_gpio)) {
> + ret = PTR_ERR(ov02a10->pd_gpio);
> + dev_err(dev, "failed to get powerdown-gpios %d\n", ret);
> + return ret;
> + }
> +
> + ov02a10->n_rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
I've asked a question about the right convention to define active low pins,
but I don't remember seeing an answer. We need to clarify this to avoid
confusion. Especially since the current binding documentation doesn't
mention anything about the sensor being active low. Let me also post a
comment to the binding patch.
> + if (IS_ERR(ov02a10->n_rst_gpio)) {
> + ret = PTR_ERR(ov02a10->n_rst_gpio);
> + dev_err(dev, "failed to get reset-gpios %d\n", ret);
> + return ret;
> + }
> +
> + for (i = 0; i < OV02A10_NUM_SUPPLIES; i++)
> + ov02a10->supplies[i].supply = ov02a10_supply_names[i];
> +
> + ret = devm_regulator_bulk_get(dev, OV02A10_NUM_SUPPLIES,
> + ov02a10->supplies);
> + if (ret) {
> + dev_err(dev, "failed to get regulators\n");
> + return ret;
> + }
> +
> + mutex_init(&ov02a10->mutex);
> + ov02a10->cur_mode = &supported_modes[0];
> + ret = ov02a10_initialize_controls(ov02a10);
> + if (ret) {
> + dev_err(dev, "failed to initialize controls\n");
> + goto err_destroy_mutex;
> + }
> +
> + ov02a10->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + ov02a10->subdev.entity.ops = &ov02a10_subdev_entity_ops;
> + ov02a10->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> + ov02a10->pad.flags = MEDIA_PAD_FL_SOURCE;
> + ret = media_entity_pads_init(&ov02a10->subdev.entity, 1, &ov02a10->pad);
> + if (ret < 0) {
> + dev_err(dev, "failed to init entity pads: %d", ret);
> + goto err_free_handler;
> + }
> +
> + pm_runtime_enable(dev);
> + if (!pm_runtime_enabled(dev)) {
> + ret = ov02a10_power_on(dev);
> + if (ret < 0) {
> + dev_err(dev, "failed to power on: %d\n", ret);
> + goto err_clean_entity;
> + }
> + }
> +
> + ret = v4l2_async_register_subdev(&ov02a10->subdev);
> + if (ret) {
> + dev_err(dev, "failed to register V4L2 subdev: %d", ret);
> + goto err_clean_entity;
> + }
> +
> + return 0;
> +
> +err_clean_entity:
Need to power off if !pm_runtime_enabled().
Best regards,
Tomasz
^ permalink raw reply
* Re: [V8, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings
From: Tomasz Figa @ 2020-05-21 19:35 UTC (permalink / raw)
To: Rob Herring
Cc: Dongchun Zhu, linus.walleij, bgolaszewski, mchehab,
andriy.shevchenko, mark.rutland, sakari.ailus, drinkcat,
matthias.bgg, bingbu.cao, srv_heupstream, linux-mediatek,
linux-arm-kernel, sj.huang, linux-media, devicetree, louis.kuo,
shengnan.wang
In-Reply-To: <20200511160207.GA32476@bogus>
Hi Rob,
On Mon, May 11, 2020 at 11:02:07AM -0500, Rob Herring wrote:
> On Sat, May 09, 2020 at 04:06:26PM +0800, Dongchun Zhu wrote:
> > Add DT bindings documentation for Omnivision OV02A10 image sensor.
> >
> > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > ---
> > .../bindings/media/i2c/ovti,ov02a10.yaml | 184 +++++++++++++++++++++
> > MAINTAINERS | 7 +
> > 2 files changed, 191 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > new file mode 100644
> > index 0000000..5468d1b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > @@ -0,0 +1,184 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright (c) 2020 MediaTek Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov02a10.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Omnivision OV02A10 CMOS Sensor Device Tree Bindings
> > +
> > +maintainers:
> > + - Dongchun Zhu <dongchun.zhu@mediatek.com>
> > +
> > +description: |-
> > + The Omnivision OV02A10 is a low-cost, high performance, 1/5-inch, 2 megapixel
> > + image sensor, which is the latest production derived from Omnivision's CMOS
> > + image sensor technology. Ihis chip supports high frame rate speeds up to 30fps
> > + @ 1600x1200 (UXGA) resolution transferred over a 1-lane MIPI interface. The
> > + sensor output is available via CSI-2 serial data output.
> > +
> > +properties:
> > + compatible:
> > + const: ovti,ov02a10
> > +
> > + reg:
> > + description: I2C device address
>
> Drop this. Nothing specific to this device.
>
> > + maxItems: 1
> > +
> > + clocks:
> > + items:
> > + - description: top mux camtg clock
> > + - description: devider clock
>
> typo
>
> > +
> > + clock-names:
> > + items:
> > + - const: eclk
> > + - const: freq_mux
> > +
> > + clock-frequency:
> > + description:
> > + Frequency of the eclk clock in Hertz.
> > +
> > + dovdd-supply:
> > + description:
> > + Definition of the regulator used as interface power supply.
> > +
> > + avdd-supply:
> > + description:
> > + Definition of the regulator used as analog power supply.
> > +
> > + dvdd-supply:
> > + description:
> > + Definition of the regulator used as digital power supply.
> > +
> > + powerdown-gpios:
> > + maxItems: 1
> > +
> > + reset-gpios:
> > + maxItems: 1
I asked a question about defining GPIO polarities some time ago, but I
guess it slipped through.
The chip documentation calls the reset pin as "RST_PAD (low level
reset)". Where should the inversion be handled, in the driver or here,
by having the DT include a necessary flag in the specifier?
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH 3/4] remoteproc: add support for a new 64-bit trace version
From: Suman Anna @ 2020-05-21 19:42 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Rob Herring, Mathieu Poirier, Clement Leger, Loic Pallardy,
Arnaud Pouliquen, Lokesh Vutla, linux-remoteproc, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <20200521180417.GJ408178@builder.lan>
Hi Bjorn,
On 5/21/20 1:04 PM, Bjorn Andersson wrote:
> On Wed 25 Mar 13:47 PDT 2020, Suman Anna wrote:
>
>> Introduce a new trace entry resource structure that accommodates
>> a 64-bit device address to support 64-bit processors. This is to
>> be used using an overloaded version value of 1 in the upper 32-bits
>> of the previous resource type field. The new resource still uses
>> 32-bits for the length field (followed by a 32-bit reserved field,
>> so can be updated in the future), which is a sufficiently large
>> trace buffer size. A 32-bit padding field also had to be added
>> to align the device address on a 64-bit boundary, and match the
>> usage on the firmware side.
>>
>> The remoteproc debugfs logic also has been adjusted accordingly.
>>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> ---
>> drivers/remoteproc/remoteproc_core.c | 40 ++++++++++++++++++++-----
>> drivers/remoteproc/remoteproc_debugfs.c | 37 ++++++++++++++++++-----
>> include/linux/remoteproc.h | 26 ++++++++++++++++
>> 3 files changed, 87 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index 53bc37c508c6..b9a097990862 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -609,21 +609,45 @@ void rproc_vdev_release(struct kref *ref)
>> *
>> * Returns 0 on success, or an appropriate error code otherwise
>> */
>> -static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
>> +static int rproc_handle_trace(struct rproc *rproc, void *rsc,
>> int offset, int avail, u16 ver)
>> {
>> struct rproc_debug_trace *trace;
>> struct device *dev = &rproc->dev;
>> + struct fw_rsc_trace *rsc1;
>> + struct fw_rsc_trace2 *rsc2;
>> char name[15];
>> + size_t rsc_size;
>> + u32 reserved;
>> + u64 da;
>> + u32 len;
>> +
>> + if (!ver) {
>
> This looks like a switch to me, but I also do think this looks rather
> crude, if you spin off the tail of this function and call it from a
> rproc_handle_trace() and rproc_handle_trace64() I believe this would be
> cleaner.
Yeah, ok. Will refactor for this in v2.
>
>> + rsc1 = (struct fw_rsc_trace *)rsc;
>> + rsc_size = sizeof(*rsc1);
>> + reserved = rsc1->reserved;
>> + da = rsc1->da;
>> + len = rsc1->len;
>> + } else if (ver == 1) {
>> + rsc2 = (struct fw_rsc_trace2 *)rsc;
>> + rsc_size = sizeof(*rsc2);
>> + reserved = rsc2->reserved;
>> + da = rsc2->da;
>> + len = rsc2->len;
>> + } else {
>> + dev_err(dev, "unsupported trace rsc version %d\n", ver);
>
> If we use "type" to describe your 64-bit-da-trace then this sanity check
> would have been taken care of by the core.
>
>> + return -EINVAL;
>> + }
>>
>> - if (sizeof(*rsc) > avail) {
>> + if (rsc_size > avail) {
>> dev_err(dev, "trace rsc is truncated\n");
>> return -EINVAL;
>> }
>>
>> /* make sure reserved bytes are zeroes */
>> - if (rsc->reserved) {
>> - dev_err(dev, "trace rsc has non zero reserved bytes\n");
>> + if (reserved) {
>> + dev_err(dev, "trace rsc has non zero reserved bytes, value = 0x%x\n",
>> + reserved);
>> return -EINVAL;
>> }
>>
>> @@ -632,8 +656,8 @@ static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
>> return -ENOMEM;
>>
>> /* set the trace buffer dma properties */
>> - trace->trace_mem.len = rsc->len;
>> - trace->trace_mem.da = rsc->da;
>> + trace->trace_mem.len = len;
>> + trace->trace_mem.da = da;
>>
>> /* set pointer on rproc device */
>> trace->rproc = rproc;
>> @@ -652,8 +676,8 @@ static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
>>
>> rproc->num_traces++;
>>
>> - dev_dbg(dev, "%s added: da 0x%x, len 0x%x\n",
>> - name, rsc->da, rsc->len);
>> + dev_dbg(dev, "%s added: da 0x%llx, len 0x%x\n",
>> + name, da, len);
>>
>> return 0;
>> }
>> diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
>> index 3560eed7a360..ff43736db45a 100644
>> --- a/drivers/remoteproc/remoteproc_debugfs.c
>> +++ b/drivers/remoteproc/remoteproc_debugfs.c
>> @@ -192,7 +192,8 @@ static int rproc_rsc_table_show(struct seq_file *seq, void *p)
>> struct resource_table *table = rproc->table_ptr;
>> struct fw_rsc_carveout *c;
>> struct fw_rsc_devmem *d;
>> - struct fw_rsc_trace *t;
>> + struct fw_rsc_trace *t1;
>> + struct fw_rsc_trace2 *t2;
>> struct fw_rsc_vdev *v;
>> int i, j;
>>
>> @@ -205,6 +206,7 @@ static int rproc_rsc_table_show(struct seq_file *seq, void *p)
>> int offset = table->offset[i];
>> struct fw_rsc_hdr *hdr = (void *)table + offset;
>> void *rsc = (void *)hdr + sizeof(*hdr);
>> + u16 ver = hdr->st.v;
>>
>> switch (hdr->st.t) {
>> case RSC_CARVEOUT:
>> @@ -230,13 +232,32 @@ static int rproc_rsc_table_show(struct seq_file *seq, void *p)
>> seq_printf(seq, " Name %s\n\n", d->name);
>> break;
>> case RSC_TRACE:
>> - t = rsc;
>> - seq_printf(seq, "Entry %d is of type %s\n",
>> - i, types[hdr->st.t]);
>> - seq_printf(seq, " Device Address 0x%x\n", t->da);
>> - seq_printf(seq, " Length 0x%x Bytes\n", t->len);
>> - seq_printf(seq, " Reserved (should be zero) [%d]\n", t->reserved);
>> - seq_printf(seq, " Name %s\n\n", t->name);
>> + if (ver == 0) {
>
> Again, this is a switch, here in a switch. Just defining a new
> RSC_TRACE64 type would reduce the amount of code here...
OK.
>
>> + t1 = rsc;
>> + seq_printf(seq, "Entry %d is version %d of type %s\n",
>> + i, ver, types[hdr->st.t]);
>> + seq_printf(seq, " Device Address 0x%x\n",
>> + t1->da);
>> + seq_printf(seq, " Length 0x%x Bytes\n",
>> + t1->len);
>> + seq_printf(seq, " Reserved (should be zero) [%d]\n",
>> + t1->reserved);
>> + seq_printf(seq, " Name %s\n\n", t1->name);
>> + } else if (ver == 1) {
>> + t2 = rsc;
>> + seq_printf(seq, "Entry %d is version %d of type %s\n",
>> + i, ver, types[hdr->st.t]);
>> + seq_printf(seq, " Device Address 0x%llx\n",
>> + t2->da);
>> + seq_printf(seq, " Length 0x%x Bytes\n",
>> + t2->len);
>> + seq_printf(seq, " Reserved (should be zero) [%d]\n",
>> + t2->reserved);
>> + seq_printf(seq, " Name %s\n\n", t2->name);
>> + } else {
>> + seq_printf(seq, "Entry %d is an unsupported version %d of type %s\n",
>> + i, ver, types[hdr->st.t]);
>> + }
>> break;
>> case RSC_VDEV:
>> v = rsc;
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 526d3cb45e37..3b3bea42f8b1 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -243,6 +243,32 @@ struct fw_rsc_trace {
>> u8 name[32];
>> } __packed;
>>
>> +/**
>> + * struct fw_rsc_trace2 - trace buffer declaration supporting 64-bits
>> + * @padding: initial padding after type field for aligned 64-bit access
>> + * @da: device address (64-bit)
>> + * @len: length (in bytes)
>> + * @reserved: reserved (must be zero)
>> + * @name: human-readable name of the trace buffer
>> + *
>> + * This resource entry is an enhanced version of the fw_rsc_trace resourec entry
>> + * and the provides equivalent functionality but designed for 64-bit remote
>> + * processors.
>> + *
>> + * @da specifies the device address of the buffer, @len specifies
>> + * its size, and @name may contain a human readable name of the trace buffer.
>> + *
>> + * After booting the remote processor, the trace buffers are exposed to the
>> + * user via debugfs entries (called trace0, trace1, etc..).
>> + */
>> +struct fw_rsc_trace2 {
>
> Sounds more like fw_rsc_trace64 to me - in particular since the version
> of trace2 is 1...
Yeah, will rename this.
>
>> + u32 padding;
>> + u64 da;
>> + u32 len;
>> + u32 reserved;
>
> What's the purpose of this reserved field?
Partly to make sure the entire resource is aligned on an 8-byte, and
partly copied over from fw_rsc_trace entry. I guess 32-bits is already
large enough of a size for trace entries irrespective of 32-bit or
64-bit traces, so I doubt if we want to make the len field also a u64.
regards
Suman
>
> Regards,
> Bjorn
>
>> + u8 name[32];
>> +} __packed;
>> +
>> /**
>> * struct fw_rsc_vdev_vring - vring descriptor entry
>> * @da: device address
>> --
>> 2.23.0
>>
^ permalink raw reply
* Re: [PATCH 2/4] remoteproc: introduce version element into resource type field
From: Bjorn Andersson @ 2020-05-21 19:41 UTC (permalink / raw)
To: Suman Anna
Cc: Rob Herring, Mathieu Poirier, Clement Leger, Loic Pallardy,
Arnaud Pouliquen, Lokesh Vutla, linux-remoteproc, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <57ae5678-fd0a-07a8-6165-a2cf7ccdef88@ti.com>
On Thu 21 May 12:29 PDT 2020, Suman Anna wrote:
> On 5/21/20 2:21 PM, Bjorn Andersson wrote:
> > On Thu 21 May 12:06 PDT 2020, Suman Anna wrote:
> >
> > > Hi Bjorn,
> > >
> > > On 5/21/20 12:54 PM, Bjorn Andersson wrote:
> > > > On Wed 25 Mar 13:46 PDT 2020, Suman Anna wrote:
> > > >
> > > > > The current remoteproc core has supported only 32-bit remote
> > > > > processors and as such some of the current resource structures
> > > > > may not scale well for 64-bit remote processors, and would
> > > > > require new versions of resource types. Each resource is currently
> > > > > identified by a 32-bit type field. Introduce the concept of version
> > > > > for these resource types by overloading this 32-bit type field
> > > > > into two 16-bit version and type fields with the existing resources
> > > > > behaving as version 0 thereby providing backward compatibility.
> > > > >
> > > > > The version field is passed as an additional argument to each of
> > > > > the handler functions, and all the existing handlers are updated
> > > > > accordingly. Each specific handler will be updated on a need basis
> > > > > when a new version of the resource type is added.
> > > > >
> > > >
> > > > I really would prefer that we add additional types for the new
> > > > structures, neither side will be compatible with new versions without
> > > > enhancements to their respective implementations anyways.
> > >
> > > OK.
> > >
> > > >
> > > > > An alternate way would be to introduce the new types as completely
> > > > > new resource types which would require additional customization of
> > > > > the resource handlers based on the 32-bit or 64-bit mode of a remote
> > > > > processor, and introduction of an additional mode flag to the rproc
> > > > > structure.
> > > > >
> > > >
> > > > What would this "mode" indicate? If it's version 0 or 1?
> > >
> > > No, for indicating if the remoteproc is 32-bit or 64-bit and adjust the
> > > loading handlers if the resource types need to be segregated accordingly.
> > >
> >
> > Sorry, I think I'm misunderstanding something. Wouldn't your 64-bit
> > remote processor need different firmware from your 32-bit processor
> > anyways, if you want to support the wider resource? And you would pack
> > your firmware with the appropriate resource types?
>
> Yes, that's correct.
>
> >
> > Afaict the bit width of your remote processor, busses or memory is
> > unrelated to the choice of number of bits used to express things in the
> > resource table.
>
> I would have to add the new resource type to the loading_handlers right, so
> it is a question of whether we want to impose any restrictions in remoteproc
> core or not from supporting a certain resource type (eg: I don't expect
> RSC_TRACE entries on 64-bit processors).
>
Right, but either you add support for new resource types to the
loading_handlers, or you add the version checks within each handler,
either way you will have to do some work to be compatible with new
versions.
Regarding what resources would be fit for a 64-bit processor probably
relates to many things, in particular the question of what we actually
mean when we say that a coprocessor is 64-bit. So I don't really see a
need for the remoteproc core to prevent someone to design their
system/firmware to have a 64-bit CPU being passed 32-bit addresses.
Regards,
Bjorn
^ permalink raw reply
* [PATCH] arm: dts: am335x-boneblack: add gpio-line-names
From: Drew Fustini @ 2020-05-21 19:50 UTC (permalink / raw)
To: Linus Walleij, Grygorii Strashko, Benoît Cousson,
Tony Lindgren, Rob Herring, Linux-OMAP, devicetree, linux-kernel,
Jason Kridner, Robert Nelson
Add gpio-line-names properties to the GPIO controller nodes.
The BeagleBone Black has P8 and P9 headers [0] which expose many of the
AM3358 ZCZ SoC balls to stacking expansion boards called "capes", or to
other external connections like jumper wires connected to a breadboard.
BeagleBone users will often refer to the "Cape Exanpsion Headers" pin
diagram [1] as it is in the "Bone101" getting started tutorial. [2]
Most of the P8 and P9 header pins can muxed to a GPIO line. The
gpio-line-names describe which P8 or P9 pin that line goes to and the
default mux for that P8 or P9 pin if it is not GPIO.
For example, gpiochip 1 line 0 is connected to P8 header pin 25 (P8_25)
however the default device tree has the corresponding BGA ball (ZCZ U7)
muxed to mmc1_dat0 as it is used for the on-board eMMC chip. For that
GPIO line to be used, one would need to modify the device tree to
disable the eMMC and change the pin mux for that ball to GPIO mode.
Some of the AM3358 ZCZ balls corresponding to GPIO lines are not routed
to a P8 or P9 header, but are instead wired to some peripheral device
like on-board eMMC, HDMI framer IC, or status LEDs. Those names are in
brackets to denote those GPIO lines can not be used.
Some GPIO lines are named "[NC]" as the corresponding balls are not
routed to anything on the PCB.
The goal for these names is to make it easier for a user viewing the
output of gpioinfo to determine which P8 or P9 pin is connected to a
GPIO line. The output of gpioinfo on a BeagleBone Black would be:
[0] https://git.io/JfgOd
[1] https://beagleboard.org/capes
[1] https://beagleboard.org/Support/bone101
[2] https://beagleboard.org/static/images/cape-headers.png
Reviewed-by: Jason Kridner <jason@beagleboard.org>
Reviewed-by: Robert Nelson <robertcnelson@gmail.com>
Signed-off-by: Drew Fustini <drew@beagleboard.org>
---
V1 note:
I had posted a patch with these line names for am335x-bone-common.dtsi
but Grygorii Strashko pointed out that the names are not applicable to
all BeagleBone models. Thus is have created this patch to add these
names just for the BeagleBone Black.
arch/arm/boot/dts/am335x-boneblack.dts | 144 +++++++++++++++++++++++++
1 file changed, 144 insertions(+)
diff --git a/arch/arm/boot/dts/am335x-boneblack.dts b/arch/arm/boot/dts/am335x-boneblack.dts
index d3928662aed4..5f31e832eb82 100644
--- a/arch/arm/boot/dts/am335x-boneblack.dts
+++ b/arch/arm/boot/dts/am335x-boneblack.dts
@@ -23,3 +23,147 @@ oppnitro-1000000000 {
opp-supported-hw = <0x06 0x0100>;
};
};
+
+&gpio0 {
+ gpio-line-names =
+ "[ethernet]",
+ "[ethernet]",
+ "P9_22 [spi0_sclk]",
+ "P9_21 [spi0_d0]",
+ "P9_18 [spi0_d1]",
+ "P9_17 [spi0_cs0]",
+ "[sd card]",
+ "P9_42A [ecappwm0]",
+ "P8_35 [hdmi]",
+ "P8_33 [hdmi]",
+ "P8_31 [hdmi]",
+ "P8_32 [hdmi]",
+ "P9_20 [i2c2_sda]",
+ "P9_19 [i2c2_scl]",
+ "P9_26 [uart1_rxd]",
+ "P9_24 [uart1_txd]",
+ "[ethernet]",
+ "[ethernet]",
+ "[usb]",
+ "[hdmi]",
+ "P9_41B",
+ "[ethernet]",
+ "P8_19 [ehrpwm2a]",
+ "P8_13 [ehrpwm2b]",
+ "[NC]",
+ "[NC]",
+ "P8_14",
+ "P8_17",
+ "[ethernet]",
+ "[ethernet]",
+ "P9_11 [uart4_rxd]",
+ "P9_13 [uart4_txd]";
+};
+
+&gpio1 {
+ gpio-line-names =
+ "P8_25 [emmc]",
+ "[emmc]",
+ "P8_5 [emmc]",
+ "P8_6 [emmc]",
+ "P8_23 [emmc]",
+ "P8_22 [emmc]",
+ "P8_3 [emmc]",
+ "P8_4 [emmc]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "P8_12",
+ "P8_11",
+ "P8_16",
+ "P8_15",
+ "P9_15A",
+ "P9_23",
+ "P9_14 [ehrpwm1a]",
+ "P9_16 [ehrpwm1b]",
+ "[emmc]",
+ "[usr0 led]",
+ "[usr1 led]",
+ "[usr2 led]",
+ "[usr3 led]",
+ "[hdmi]",
+ "[usb]",
+ "[hdmi audio]",
+ "P9_12",
+ "P8_26",
+ "P8_21 [emmc]",
+ "P8_20 [emmc]";
+};
+
+&gpio2 {
+ gpio-line-names =
+ "P9_15B",
+ "P8_18",
+ "P8_7",
+ "P8_8",
+ "P8_10",
+ "P8_9",
+ "P8_45 [hdmi]",
+ "P8_46 [hdmi]",
+ "P8_43 [hdmi]",
+ "P8_44 [hdmi]",
+ "P8_41 [hdmi]",
+ "P8_42 [hdmi]",
+ "P8_39 [hdmi]",
+ "P8_40 [hdmi]",
+ "P8_37 [hdmi]",
+ "P8_38 [hdmi]",
+ "P8_36 [hdmi]",
+ "P8_34 [hdmi]",
+ "[ethernet]",
+ "[ethernet]",
+ "[ethernet]",
+ "[ethernet]",
+ "P8_27 [hdmi]",
+ "P8_29 [hdmi]",
+ "P8_28 [hdmi]",
+ "P8_30 [hdmi]",
+ "[emmc]",
+ "[emmc]",
+ "[emmc]",
+ "[emmc]",
+ "[emmc]",
+ "[emmc]";
+};
+
+&gpio3 {
+ gpio-line-names =
+ "[ethernet]",
+ "[ethernet]",
+ "[ethernet]",
+ "[ethernet]",
+ "[ethernet]",
+ "[i2c0]",
+ "[i2c0]",
+ "[emu]",
+ "[emu]",
+ "[ethernet]",
+ "[ethernet]",
+ "[NC]",
+ "[NC]",
+ "[usb]",
+ "P9_31 [spi1_sclk]",
+ "P9_29 [spi1_d0]",
+ "P9_30 [spi1_d1]",
+ "P9_28 [spi1_cs0]",
+ "P9_42B [ecappwm0]",
+ "P9_27",
+ "P9_41A",
+ "P9_25",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]";
+};
--
2.25.1
^ permalink raw reply related
* Re: [V6, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
From: Tomasz Figa @ 2020-05-21 19:51 UTC (permalink / raw)
To: Dongchun Zhu
Cc: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt,
mark.rutland, sakari.ailus, drinkcat, matthias.bgg, bingbu.cao,
srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang,
linux-media, devicetree, louis.kuo, shengnan.wang
In-Reply-To: <20200518132731.20855-3-dongchun.zhu@mediatek.com>
Hi Dongchun, Sakari,
On Mon, May 18, 2020 at 09:27:31PM +0800, Dongchun Zhu wrote:
> Add a V4L2 sub-device driver for DW9768 voice coil motor, providing
> control to set the desired focus via IIC serial interface.
>
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> ---
> MAINTAINERS | 1 +
> drivers/media/i2c/Kconfig | 13 ++
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/dw9768.c | 515 +++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 530 insertions(+)
> create mode 100644 drivers/media/i2c/dw9768.c
[snip]
> +/*
> + * DW9768_AAC_PRESC_REG & DW9768_AAC_TIME_REG determine VCM operation time.
> + * If DW9768_AAC_PRESC_REG is 0x41, and DW9768_AAC_TIME_REG is 0x39, VCM mode
> + * would be AAC3, Operation Time would be 0.70xTvib, that is 8.40ms.
> + */
> +#define DW9768_MOVE_DELAY_US 8400
> +#define DW9768_STABLE_TIME_US 20000
These times are only valid with the specific settings mentioned in the
comment. If one sets different settings in DT, the driver would apply
incorrect delays. Rather than hardcoded, they should be computed based
on the configured values.
That said, I wonder if we're not digging too deep now. Sakari, do you
think we could take a step back, remove the optional DT properties and
just support the fixed values for now, so that we can get a basic driver
upstream first without doubling the effort?
> +
> +static const char * const dw9768_supply_names[] = {
> + "vin", /* I2C I/O interface power */
> + "vdd", /* VCM power */
> +};
> +
> +/* dw9768 device structure */
> +struct dw9768 {
> + struct regulator_bulk_data supplies[ARRAY_SIZE(dw9768_supply_names)];
> + struct v4l2_ctrl_handler ctrls;
> + struct v4l2_ctrl *focus;
> + struct v4l2_subdev sd;
> +
> + u32 aac_mode;
> + u32 aac_timing;
> + u32 clock_dividing_rate;
> + bool aac_mode_control_enable;
> + bool aact_cnt_select_enable;
> + bool clock_dividing_rate_select_enable;
nit: Separate types from names with just 1 space.
> +};
> +
> +static inline struct dw9768 *sd_to_dw9768(struct v4l2_subdev *subdev)
> +{
> + return container_of(subdev, struct dw9768, sd);
> +}
> +
> +struct regval_list {
> + u8 reg_num;
> + u8 value;
> +};
> +
> +static int dw9768_read_smbus(struct dw9768 *dw9768, unsigned char reg,
> + unsigned char *val)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> + int ret;
> +
> + ret = i2c_smbus_read_byte_data(client, reg);
> +
> + if (ret < 0)
> + return ret;
> +
> + *val = (unsigned char)ret;
> +
> + return 0;
> +}
Why do we need this function? Couldn't we just call
i2c_smbus_read_byte_data() directly?
[snip]
> +static int dw9768_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct dw9768 *dw9768;
> + unsigned int aac_mode_select;
> + unsigned int aac_timing_select;
> + unsigned int clock_dividing_rate_select;
> + unsigned int i;
> + int ret;
> +
> + dw9768 = devm_kzalloc(dev, sizeof(*dw9768), GFP_KERNEL);
> + if (!dw9768)
> + return -ENOMEM;
> +
> + v4l2_i2c_subdev_init(&dw9768->sd, client, &dw9768_ops);
> + dw9768->aac_mode_control_enable = false;
> + dw9768->aact_cnt_select_enable = false;
> + dw9768->clock_dividing_rate_select_enable = false;
devm_kzalloc() initializes the memory to zero, so no need to set anything
to false explicitly.
> +
> + /* Optional indication of AAC mode select */
> + ret = fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-mode",
> + &aac_mode_select);
> +
> + if (!ret) {
> + dw9768->aac_mode_control_enable = true;
> + dw9768->aac_mode = aac_mode_select;
How about making aac_mode a signed int and assigning -1 by
default? Then we don't need two separate fields in the struct.
> + }
> +
> + /* Optional indication of VCM internal clock dividing rate select */
> + ret = fwnode_property_read_u32(dev_fwnode(dev),
> + "dongwoon,clock-dividing-rate",
> + &clock_dividing_rate_select);
> +
> + if (!ret) {
> + dw9768->clock_dividing_rate_select_enable = true;
> + dw9768->clock_dividing_rate = clock_dividing_rate_select;
Ditto.
> + }
> +
> + /* Optional indication of AAC Timing */
> + ret = fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-timing",
> + &aac_timing_select);
> +
> + if (!ret) {
> + dw9768->aact_cnt_select_enable = true;
> + dw9768->aac_timing = aac_timing_select;
Ditto.
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(dw9768_supply_names); i++)
> + dw9768->supplies[i].supply = dw9768_supply_names[i];
> +
> + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(dw9768_supply_names),
> + dw9768->supplies);
> + if (ret) {
> + dev_err(dev, "failed to get regulators\n");
> + return ret;
> + }
> +
> + ret = dw9768_init_controls(dw9768);
> + if (ret)
> + goto entity_cleanup;
> +
> + dw9768->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + dw9768->sd.internal_ops = &dw9768_int_ops;
> +
> + ret = media_entity_pads_init(&dw9768->sd.entity, 0, NULL);
> + if (ret < 0)
> + goto entity_cleanup;
> +
> + dw9768->sd.entity.function = MEDIA_ENT_F_LENS;
> +
> + pm_runtime_enable(dev);
> + if (!pm_runtime_enabled(dev)) {
> + ret = dw9768_runtime_resume(dev);
> + if (ret < 0) {
> + dev_err(dev, "failed to power on: %d\n", ret);
> + goto entity_cleanup;
> + }
> + }
> +
> + ret = v4l2_async_register_subdev(&dw9768->sd);
> + if (ret < 0)
> + goto entity_cleanup;
> +
> + return 0;
> +
> +entity_cleanup:
Need to power off if the code above powered on.
> + v4l2_ctrl_handler_free(&dw9768->ctrls);
> + media_entity_cleanup(&dw9768->sd.entity);
> + return ret;
> +}
> +
> +static int dw9768_remove(struct i2c_client *client)
> +{
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct dw9768 *dw9768 = sd_to_dw9768(sd);
> +
> + pm_runtime_disable(&client->dev);
First the device must be unregistered from the userspace. Otherwise there
is a race condition that risks the userspace accessing the device while the
deinitialization is happening.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH 2/4] remoteproc: introduce version element into resource type field
From: Suman Anna @ 2020-05-21 19:52 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Rob Herring, Mathieu Poirier, Clement Leger, Loic Pallardy,
Arnaud Pouliquen, Lokesh Vutla, linux-remoteproc, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <20200521194116.GP408178@builder.lan>
On 5/21/20 2:41 PM, Bjorn Andersson wrote:
> On Thu 21 May 12:29 PDT 2020, Suman Anna wrote:
>
>> On 5/21/20 2:21 PM, Bjorn Andersson wrote:
>>> On Thu 21 May 12:06 PDT 2020, Suman Anna wrote:
>>>
>>>> Hi Bjorn,
>>>>
>>>> On 5/21/20 12:54 PM, Bjorn Andersson wrote:
>>>>> On Wed 25 Mar 13:46 PDT 2020, Suman Anna wrote:
>>>>>
>>>>>> The current remoteproc core has supported only 32-bit remote
>>>>>> processors and as such some of the current resource structures
>>>>>> may not scale well for 64-bit remote processors, and would
>>>>>> require new versions of resource types. Each resource is currently
>>>>>> identified by a 32-bit type field. Introduce the concept of version
>>>>>> for these resource types by overloading this 32-bit type field
>>>>>> into two 16-bit version and type fields with the existing resources
>>>>>> behaving as version 0 thereby providing backward compatibility.
>>>>>>
>>>>>> The version field is passed as an additional argument to each of
>>>>>> the handler functions, and all the existing handlers are updated
>>>>>> accordingly. Each specific handler will be updated on a need basis
>>>>>> when a new version of the resource type is added.
>>>>>>
>>>>>
>>>>> I really would prefer that we add additional types for the new
>>>>> structures, neither side will be compatible with new versions without
>>>>> enhancements to their respective implementations anyways.
>>>>
>>>> OK.
>>>>
>>>>>
>>>>>> An alternate way would be to introduce the new types as completely
>>>>>> new resource types which would require additional customization of
>>>>>> the resource handlers based on the 32-bit or 64-bit mode of a remote
>>>>>> processor, and introduction of an additional mode flag to the rproc
>>>>>> structure.
>>>>>>
>>>>>
>>>>> What would this "mode" indicate? If it's version 0 or 1?
>>>>
>>>> No, for indicating if the remoteproc is 32-bit or 64-bit and adjust the
>>>> loading handlers if the resource types need to be segregated accordingly.
>>>>
>>>
>>> Sorry, I think I'm misunderstanding something. Wouldn't your 64-bit
>>> remote processor need different firmware from your 32-bit processor
>>> anyways, if you want to support the wider resource? And you would pack
>>> your firmware with the appropriate resource types?
>>
>> Yes, that's correct.
>>
>>>
>>> Afaict the bit width of your remote processor, busses or memory is
>>> unrelated to the choice of number of bits used to express things in the
>>> resource table.
>>
>> I would have to add the new resource type to the loading_handlers right, so
>> it is a question of whether we want to impose any restrictions in remoteproc
>> core or not from supporting a certain resource type (eg: I don't expect
>> RSC_TRACE entries on 64-bit processors).
>>
>
> Right, but either you add support for new resource types to the
> loading_handlers, or you add the version checks within each handler,
> either way you will have to do some work to be compatible with new
> versions.
>
> Regarding what resources would be fit for a 64-bit processor probably
> relates to many things, in particular the question of what we actually
> mean when we say that a coprocessor is 64-bit. So I don't really see a
> need for the remoteproc core to prevent someone to design their
> system/firmware to have a 64-bit CPU being passed 32-bit addresses.
OK. In general, I have seen firmware developers get confused w.r.t the
resource types, that's why I was inclined to go with the restrictive
checking. Anyway, will rework the support as per the comments.
regards
Suman
^ permalink raw reply
* Re: [PATCH 02/12] ACPI/IORT: Make iort_get_device_domain IRQ domain agnostic
From: Bjorn Helgaas @ 2020-05-21 19:56 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: linux-arm-kernel, Will Deacon, Hanjun Guo, Bjorn Helgaas,
Sudeep Holla, Catalin Marinas, Robin Murphy, Rafael J. Wysocki,
Marc Zyngier, iommu, linux-acpi, devicetree, linux-pci,
Rob Herring, Joerg Roedel, Makarand Pawagi, Diana Craciun,
Laurentiu Tudor
In-Reply-To: <20200521130008.8266-3-lorenzo.pieralisi@arm.com>
On Thu, May 21, 2020 at 01:59:58PM +0100, Lorenzo Pieralisi wrote:
> iort_get_device_domain() is PCI specific but it need not be,
> since it can be used to retrieve IRQ domain nexus of any kind
> by adding an irq_domain_bus_token input to it.
>
> Make it PCI agnostic by also renaming the requestor ID input
> to a more generic ID name.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Hanjun Guo <guohanjun@huawei.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Marc Zyngier <maz@kernel.org>
Acked-by: Bjorn Helgaas <bhelgaas@google.com> # pci/msi.c
> ---
> drivers/acpi/arm64/iort.c | 14 +++++++-------
> drivers/pci/msi.c | 3 ++-
> include/linux/acpi_iort.h | 7 ++++---
> 3 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 7cfd77b5e6e8..8f2a961c1364 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -567,7 +567,6 @@ static struct acpi_iort_node *iort_find_dev_node(struct device *dev)
> node = iort_get_iort_node(dev->fwnode);
> if (node)
> return node;
> -
> /*
> * if not, then it should be a platform device defined in
> * DSDT/SSDT (with Named Component node in IORT)
> @@ -658,13 +657,13 @@ static int __maybe_unused iort_find_its_base(u32 its_id, phys_addr_t *base)
> /**
> * iort_dev_find_its_id() - Find the ITS identifier for a device
> * @dev: The device.
> - * @req_id: Device's requester ID
> + * @id: Device's ID
> * @idx: Index of the ITS identifier list.
> * @its_id: ITS identifier.
> *
> * Returns: 0 on success, appropriate error value otherwise
> */
> -static int iort_dev_find_its_id(struct device *dev, u32 req_id,
> +static int iort_dev_find_its_id(struct device *dev, u32 id,
> unsigned int idx, int *its_id)
> {
> struct acpi_iort_its_group *its;
> @@ -674,7 +673,7 @@ static int iort_dev_find_its_id(struct device *dev, u32 req_id,
> if (!node)
> return -ENXIO;
>
> - node = iort_node_map_id(node, req_id, NULL, IORT_MSI_TYPE);
> + node = iort_node_map_id(node, id, NULL, IORT_MSI_TYPE);
> if (!node)
> return -ENXIO;
>
> @@ -697,19 +696,20 @@ static int iort_dev_find_its_id(struct device *dev, u32 req_id,
> *
> * Returns: the MSI domain for this device, NULL otherwise
> */
> -struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id)
> +struct irq_domain *iort_get_device_domain(struct device *dev, u32 id,
> + enum irq_domain_bus_token bus_token)
> {
> struct fwnode_handle *handle;
> int its_id;
>
> - if (iort_dev_find_its_id(dev, req_id, 0, &its_id))
> + if (iort_dev_find_its_id(dev, id, 0, &its_id))
> return NULL;
>
> handle = iort_find_domain_token(its_id);
> if (!handle)
> return NULL;
>
> - return irq_find_matching_fwnode(handle, DOMAIN_BUS_PCI_MSI);
> + return irq_find_matching_fwnode(handle, bus_token);
> }
>
> static void iort_set_device_domain(struct device *dev,
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 6b43a5455c7a..74a91f52ecc0 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1558,7 +1558,8 @@ struct irq_domain *pci_msi_get_device_domain(struct pci_dev *pdev)
> pci_for_each_dma_alias(pdev, get_msi_id_cb, &rid);
> dom = of_msi_map_get_device_domain(&pdev->dev, rid);
> if (!dom)
> - dom = iort_get_device_domain(&pdev->dev, rid);
> + dom = iort_get_device_domain(&pdev->dev, rid,
> + DOMAIN_BUS_PCI_MSI);
> return dom;
> }
> #endif /* CONFIG_PCI_MSI_IRQ_DOMAIN */
> diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
> index 8e7e2ec37f1b..08ec6bd2297f 100644
> --- a/include/linux/acpi_iort.h
> +++ b/include/linux/acpi_iort.h
> @@ -29,7 +29,8 @@ struct fwnode_handle *iort_find_domain_token(int trans_id);
> #ifdef CONFIG_ACPI_IORT
> void acpi_iort_init(void);
> u32 iort_msi_map_rid(struct device *dev, u32 req_id);
> -struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id);
> +struct irq_domain *iort_get_device_domain(struct device *dev, u32 id,
> + enum irq_domain_bus_token bus_token);
> void acpi_configure_pmsi_domain(struct device *dev);
> int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id);
> /* IOMMU interface */
> @@ -40,8 +41,8 @@ int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head);
> static inline void acpi_iort_init(void) { }
> static inline u32 iort_msi_map_rid(struct device *dev, u32 req_id)
> { return req_id; }
> -static inline struct irq_domain *iort_get_device_domain(struct device *dev,
> - u32 req_id)
> +static inline struct irq_domain *iort_get_device_domain(
> + struct device *dev, u32 id, enum irq_domain_bus_token bus_token)
> { return NULL; }
> static inline void acpi_configure_pmsi_domain(struct device *dev) { }
> /* IOMMU interface */
> --
> 2.26.1
>
^ permalink raw reply
* Re: [PATCH v9 1/2] PCI: Microchip: Add host driver for Microchip PCIe controller
From: robh @ 2020-05-21 19:56 UTC (permalink / raw)
To: Daire.McNamara
Cc: amurray, devicetree, linux-pci, robh-dt, lorenzo.pieralisi,
bhelgaas
In-Reply-To: <5dc3002680da40400b083748329d8b736219952e.camel@microchip.com>
On Wed, 20 May 2020 11:44:36 +0000, Daire.McNamara@microchip.com wrote:
>
> This patch adds device tree bindings for the Microchip
> PCIe PolarFire PCIe controller when configured in
> host (Root Complex) mode.
>
> Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> ---
> .../bindings/pci/microchip,pcie-host.yaml | 94 +++++++++++++++++++
> 1 file changed, 94 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
>
My bot found errors running 'make dt_binding_check' on your patch:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml: properties:interrupts: {'minItems': 1, 'maxItems': 1, 'items': [{'description': 'PCIe host controller and builtin MSI controller'}]} is not valid under any of the given schemas (Possible causes of the failure):
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml: properties:interrupts: 'minItems' is not one of ['description', 'deprecated', 'const', 'enum', 'minimum', 'maximum', 'default', '$ref']
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml: properties:interrupts:maxItems: 1 is less than the minimum of 2
Documentation/devicetree/bindings/Makefile:12: recipe for target 'Documentation/devicetree/bindings/pci/microchip,pcie-host.example.dts' failed
make[1]: *** [Documentation/devicetree/bindings/pci/microchip,pcie-host.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml: ignoring, error in schema: properties: interrupts
warning: no schema found in file: ./Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml: ignoring, error in schema: properties: interrupts
warning: no schema found in file: ./Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
Makefile:1300: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2
See https://patchwork.ozlabs.org/patch/1294277
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:
pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade
Please check and re-submit.
^ permalink raw reply
* Re: [PATCH 08/12] of/irq: make of_msi_map_get_device_domain() bus agnostic
From: Bjorn Helgaas @ 2020-05-21 19:57 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: linux-arm-kernel, Diana Craciun, Bjorn Helgaas, Rob Herring,
Marc Zyngier, iommu, linux-acpi, devicetree, linux-pci,
Rafael J. Wysocki, Joerg Roedel, Hanjun Guo, Sudeep Holla,
Robin Murphy, Catalin Marinas, Will Deacon, Makarand Pawagi,
Laurentiu Tudor
In-Reply-To: <20200521130008.8266-9-lorenzo.pieralisi@arm.com>
On Thu, May 21, 2020 at 02:00:04PM +0100, Lorenzo Pieralisi wrote:
> From: Diana Craciun <diana.craciun@oss.nxp.com>
>
> of_msi_map_get_device_domain() is PCI specific but it need not be and
> can be easily changed to be bus agnostic in order to be used by other
> busses by adding an IRQ domain bus token as an input parameter.
>
> Signed-off-by: Diana Craciun <diana.craciun@oss.nxp.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Marc Zyngier <maz@kernel.org>
Acked-by: Bjorn Helgaas <bhelgaas@google.com> # pci/msi.c
> ---
> drivers/of/irq.c | 8 +++++---
> drivers/pci/msi.c | 2 +-
> include/linux/of_irq.h | 5 +++--
> 3 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index a296eaf52a5b..48a40326984f 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -613,18 +613,20 @@ u32 of_msi_map_rid(struct device *dev, struct device_node *msi_np, u32 rid_in)
> * of_msi_map_get_device_domain - Use msi-map to find the relevant MSI domain
> * @dev: device for which the mapping is to be done.
> * @rid: Requester ID for the device.
> + * @bus_token: Bus token
> *
> * Walk up the device hierarchy looking for devices with a "msi-map"
> * property.
> *
> * Returns: the MSI domain for this device (or NULL on failure)
> */
> -struct irq_domain *of_msi_map_get_device_domain(struct device *dev, u32 rid)
> +struct irq_domain *of_msi_map_get_device_domain(struct device *dev, u32 id,
> + u32 bus_token)
> {
> struct device_node *np = NULL;
>
> - __of_msi_map_rid(dev, &np, rid);
> - return irq_find_matching_host(np, DOMAIN_BUS_PCI_MSI);
> + __of_msi_map_rid(dev, &np, id);
> + return irq_find_matching_host(np, bus_token);
> }
>
> /**
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 74a91f52ecc0..9532e1d12d3f 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1556,7 +1556,7 @@ struct irq_domain *pci_msi_get_device_domain(struct pci_dev *pdev)
> u32 rid = pci_dev_id(pdev);
>
> pci_for_each_dma_alias(pdev, get_msi_id_cb, &rid);
> - dom = of_msi_map_get_device_domain(&pdev->dev, rid);
> + dom = of_msi_map_get_device_domain(&pdev->dev, rid, DOMAIN_BUS_PCI_MSI);
> if (!dom)
> dom = iort_get_device_domain(&pdev->dev, rid,
> DOMAIN_BUS_PCI_MSI);
> diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
> index 1214cabb2247..7142a3722758 100644
> --- a/include/linux/of_irq.h
> +++ b/include/linux/of_irq.h
> @@ -52,7 +52,8 @@ extern struct irq_domain *of_msi_get_domain(struct device *dev,
> struct device_node *np,
> enum irq_domain_bus_token token);
> extern struct irq_domain *of_msi_map_get_device_domain(struct device *dev,
> - u32 rid);
> + u32 id,
> + u32 bus_token);
> extern void of_msi_configure(struct device *dev, struct device_node *np);
> u32 of_msi_map_rid(struct device *dev, struct device_node *msi_np, u32 rid_in);
> #else
> @@ -85,7 +86,7 @@ static inline struct irq_domain *of_msi_get_domain(struct device *dev,
> return NULL;
> }
> static inline struct irq_domain *of_msi_map_get_device_domain(struct device *dev,
> - u32 rid)
> + u32 id, u32 bus_token)
> {
> return NULL;
> }
> --
> 2.26.1
>
^ permalink raw reply
* Re: [PATCH v2 2/9] dt-bindings: gpio: Convert mrvl-gpio to json-schema
From: robh @ 2020-05-21 19:57 UTC (permalink / raw)
To: Lubomir Rintel
Cc: Alessandro Zummo, Daniel Lezcano, Marc Zyngier, Thomas Gleixner,
Jason Cooper, linux-kernel, Bartosz Golaszewski, devicetree,
Ulf Hansson, Linus Walleij, Alexandre Belloni, Rob Herring
In-Reply-To: <20200521091356.2211020-3-lkundrak@v3.sk>
On Thu, 21 May 2020 11:13:49 +0200, Lubomir Rintel wrote:
> This converts the mrvl-gpio binding to DT schema format using json-schema.
>
> Various fixes were done during the conversion, such as adding more
> properties that are in fact mandatory or extending the examples to
> include child nodes with extra GPIO blocks.
>
> The compatible strings are a mess. It is not clear why so many of them
> are needed; the driver doesn't really seem to differentiate between the
> models. Some of them, like marvell,pxa93x-gpio and marvell,pxa1928-gpio
> are not used at all, so it's not known how many interrupts they utilize.
> On the other hand, mrvl,pxa-gpio has been seen in the tree, but it
> doesn't end up in any actual DTB file.
>
> In any case -- the schema merely copies whatever was in the original
> binding document, so it's hopefully no more wrong that the original.
>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
>
> ---
> Changes since v1:
> - Drop marvell,pxa1928-gpio
> - Drop ranges from example with no gcb child nodes
> - Add default GPL-2.0-only license tag
> - Fill in maintainers from MAINTAINERS file
>
> .../devicetree/bindings/gpio/mrvl-gpio.txt | 48 -----
> .../devicetree/bindings/gpio/mrvl-gpio.yaml | 174 ++++++++++++++++++
> 2 files changed, 174 insertions(+), 48 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/gpio/mrvl-gpio.txt
> create mode 100644 Documentation/devicetree/bindings/gpio/mrvl-gpio.yaml
>
My bot found errors running 'make dt_binding_check' on your patch:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/gpio/mrvl-gpio.example.dt.yaml: gpio@40e00000: 'ranges' is a required property
See https://patchwork.ozlabs.org/patch/1295044
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:
pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade
Please check and re-submit.
^ permalink raw reply
* Re: [PATCH v12 1/3] dt-bindings: i2c: npcm7xx: add NPCM I2C controller
From: robh @ 2020-05-21 19:58 UTC (permalink / raw)
To: Tali Perry
Cc: avifishman70, linux-i2c, linux-arm-kernel, yuenn, wsa,
andriy.shevchenko, devicetree, kfting, venture, brendanhiggins,
ofery, benjaminfair, openbmc, tmaimon77, robh+dt, linux-kernel
In-Reply-To: <20200521110910.45518-2-tali.perry1@gmail.com>
On Thu, 21 May 2020 14:09:08 +0300, Tali Perry wrote:
> Added device tree binding documentation for Nuvoton BMC
> NPCM I2C controller.
>
> Signed-off-by: Tali Perry <tali.perry1@gmail.com>
> ---
> .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml | 62 +++++++++++++++++++
> 1 file changed, 62 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
>
My bot found errors running 'make dt_binding_check' on your patch:
Error: Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.example.dts:22.28-29 syntax error
FATAL ERROR: Unable to parse input tree
scripts/Makefile.lib:312: recipe for target 'Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.example.dt.yaml' failed
make[1]: *** [Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.example.dt.yaml] Error 1
make[1]: *** Waiting for unfinished jobs....
Makefile:1300: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2
See https://patchwork.ozlabs.org/patch/1295115
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:
pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade
Please check and re-submit.
^ permalink raw reply
* Re: [PATCH v8 5/5] dt-bindings: arm: fsl: add different Protonic boards
From: robh @ 2020-05-21 20:00 UTC (permalink / raw)
To: Oleksij Rempel
Cc: Shawn Guo, Mark Rutland, Fabio Estevam, devicetree,
linux-arm-kernel, Sascha Hauer, Pengutronix Kernel Team,
NXP Linux Team, Rob Herring, linux-kernel
In-Reply-To: <20200520154116.12909-6-o.rempel@pengutronix.de>
On Wed, 20 May 2020 17:41:16 +0200, Oleksij Rempel wrote:
> Add Protonic PRTI6Q, WD2, RVT, VT7 boards.
>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
> Documentation/devicetree/bindings/arm/fsl.yaml | 4 ++++
> 1 file changed, 4 insertions(+)
>
Please add Acked-by/Reviewed-by tags when posting new versions. However,
there's no need to repost patches *only* to add the tags. The upstream
maintainer will do that for acks received on the version they apply.
If a tag was not added on purpose, please state why and what changed.
^ permalink raw reply
* Re: [PATCH v4 1/2] dt-bindings: mfd: Add ENE KB3930 Embedded Controller binding
From: robh @ 2020-05-21 20:00 UTC (permalink / raw)
To: Lubomir Rintel
Cc: Mark Rutland, Lee Jones, linux-kernel, Rob Herring, devicetree
In-Reply-To: <20200521115407.2249702-2-lkundrak@v3.sk>
On Thu, 21 May 2020 13:54:06 +0200, Lubomir Rintel wrote:
> Add binding document for the ENE KB3930 Embedded Controller.
>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
>
> ---
> Changes since v1:
> - Addressed binding validation failure
> ---
> .../devicetree/bindings/mfd/ene-kb3930.yaml | 55 +++++++++++++++++++
> 1 file changed, 55 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/ene-kb3930.yaml
>
Please add Acked-by/Reviewed-by tags when posting new versions. However,
there's no need to repost patches *only* to add the tags. The upstream
maintainer will do that for acks received on the version they apply.
If a tag was not added on purpose, please state why and what changed.
^ permalink raw reply
* Re: [PATCH 5/5] dt-bindings: timer: Add CLINT bindings
From: Sean Anderson @ 2020-05-21 20:05 UTC (permalink / raw)
To: Anup Patel, Palmer Dabbelt, Paul Walmsley, Albert Ou, Rob Herring,
Daniel Lezcano, Thomas Gleixner
Cc: devicetree, Damien Le Moal, Anup Patel, linux-kernel, Atish Patra,
Alistair Francis, linux-riscv
In-Reply-To: <20200521134544.816918-6-anup.patel@wdc.com>
On 5/21/20 9:45 AM, Anup Patel wrote:
> We add DT bindings documentation for CLINT device.
>
> Signed-off-by: Anup Patel <anup.patel@wdc.com>
> ---
> .../bindings/timer/sifive,clint.txt | 33 +++++++++++++++++++
> 1 file changed, 33 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/timer/sifive,clint.txt
>
> diff --git a/Documentation/devicetree/bindings/timer/sifive,clint.txt b/Documentation/devicetree/bindings/timer/sifive,clint.txt
> new file mode 100644
> index 000000000000..cae2dad1223a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/sifive,clint.txt
> @@ -0,0 +1,33 @@
> +SiFive Core Local Interruptor (CLINT)
> +-------------------------------------
> +
> +SiFive (and other RISC-V) SOCs include an implementation of the SiFive Core
> +Local Interruptor (CLINT) for M-mode timer and inter-processor interrupts.
> +
> +It directly connects to the timer and inter-processor interrupt lines of
> +various HARTs (or CPUs) so RISC-V per-HART (or per-CPU) local interrupt
> +controller is the parent interrupt controller for CLINT device.
> +
> +The clock frequency of CLINT is specified via "timebase-frequency" DT
> +property of "/cpus" DT node. The "timebase-frequency" DT property is
> +described in: Documentation/devicetree/bindings/riscv/cpus.yaml
> +
> +Required properties:
> +- compatible : "sifive,clint-1.0.0" and a string identifying the actual
> + detailed implementation in case that specific bugs need to be worked around.
Should the "riscv,clint0" compatible string be documented here? This
peripheral is not really specific to sifive, as it is present in most
rocket-chip cores.
> +- reg : Should contain 1 register range (address and length).
> +- interrupts-extended : Specifies which HARTs (or CPUs) are connected to
> + the CLINT. Each node pointed to should be a riscv,cpu-intc node, which
> + has a riscv node as parent.
> +
> +Example:
> +
> + clint@2000000 {
> + compatible = "sifive,clint-1.0.0", "sifive,fu540-c000-clint";
> + interrupts-extended = <
> + &cpu1-intc 3 &cpu1-intc 7
> + &cpu2-intc 3 &cpu2-intc 7
> + &cpu3-intc 3 &cpu3-intc 7
> + &cpu4-intc 3 &cpu4-intc 7>;
> + reg = <0x2000000 0x4000000>;
> + };
>
--Sean
^ permalink raw reply
* [PATCH v2] arm: dts: am335x-boneblack: add gpio-line-names
From: Drew Fustini @ 2020-05-21 20:09 UTC (permalink / raw)
To: Linus Walleij, Grygorii Strashko, Benoît Cousson,
Tony Lindgren, Rob Herring, Linux-OMAP, devicetree, linux-kernel,
Jason Kridner, Robert Nelson
The BeagleBone Black has P8 and P9 headers [0] which expose many of the
AM3358 ZCZ SoC balls to stacking expansion boards called "capes", or to
other external connections like jumper wires connected to a breadboard.
BeagleBone users will often refer to the "Cape Exanpsion Headers" pin
diagram [1] as it is in the "Bone101" getting started tutorial. [2]
Most of the P8 and P9 header pins can muxed to a GPIO line. The
gpio-line-names describe which P8 or P9 pin that line goes to and the
default mux for that P8 or P9 pin if it is not GPIO.
For example, gpiochip 1 line 0 is connected to P8 header pin 25 (P8_25)
however the default device tree has the corresponding BGA ball (ZCZ U7)
muxed to mmc1_dat0 as it is used for the on-board eMMC chip. For that
GPIO line to be used, one would need to modify the device tree to
disable the eMMC and change the pin mux for that ball to GPIO mode.
Some of the AM3358 ZCZ balls corresponding to GPIO lines are not routed
to a P8 or P9 header, but are instead wired to some peripheral device
like on-board eMMC, HDMI framer IC, or status LEDs. Those names are in
brackets to denote those GPIO lines can not be used.
Some GPIO lines are named "[NC]" as the corresponding balls are not
routed to anything on the PCB.
The goal for these names is to make it easier for a user viewing the
output of gpioinfo to determine which P8 or P9 pin is connected to a
GPIO line. The output of gpioinfo on a BeagleBone Black would be:
gpiochip0 - 32 lines:
line 0: "[ethernet]" unused input active-high
line 1: "[ethernet]" unused input active-high
line 2: "P9_22 [spi0_sclk]" unused input active-high
line 3: "P9_21 [spi0_d0]" unused input active-high
line 4: "P9_18 [spi0_d1]" unused input active-high
line 5: "P9_17 [spi0_cs0]" unused input active-high
line 6: "[sd card]" "cd" input active-low [used]
line 7: "P9_42A [ecappwm0]" unused input active-high
line 8: "P8_35 [hdmi]" unused input active-high
line 9: "P8_33 [hdmi]" unused input active-high
line 10: "P8_31 [hdmi]" unused input active-high
line 11: "P8_32 [hdmi]" unused input active-high
line 12: "P9_20 [i2c2_sda]" unused input active-high
line 13: "P9_19 [i2c2_scl]" unused input active-high
line 14: "P9_26 [uart1_rxd]" unused input active-high
line 15: "P9_24 [uart1_txd]" unused input active-high
line 16: "[ethernet]" unused input active-high
line 17: "[ethernet]" unused input active-high
line 18: "[usb]" unused input active-high
line 19: "[hdmi]" unused input active-high
line 20: "P9_41B" unused input active-high
line 21: "[ethernet]" unused input active-high
line 22: "P8_19 [ehrpwm2a]" unused input active-high
line 23: "P8_13 [ehrpwm2b]" unused input active-high
line 24: "[NC]" unused input active-high
line 25: "[NC]" unused input active-high
line 26: "P8_14" unused input active-high
line 27: "P8_17" unused input active-high
line 28: "[ethernet]" unused input active-high
line 29: "[ethernet]" unused input active-high
line 30: "P9_11 [uart4_rxd]" unused input active-high
line 31: "P9_13 [uart4_txd]" unused input active-high
gpiochip1 - 32 lines:
line 0: "P8_25 [emmc]" unused input active-high
line 1: "[emmc]" unused input active-high
line 2: "P8_5 [emmc]" unused input active-high
line 3: "P8_6 [emmc]" unused input active-high
line 4: "P8_23 [emmc]" unused input active-high
line 5: "P8_22 [emmc]" unused input active-high
line 6: "P8_3 [emmc]" unused input active-high
line 7: "P8_4 [emmc]" unused input active-high
line 8: "[NC]" unused input active-high
line 9: "[NC]" unused input active-high
line 10: "[NC]" unused input active-high
line 11: "[NC]" unused input active-high
line 12: "P8_12" unused input active-high
line 13: "P8_11" unused input active-high
line 14: "P8_16" unused input active-high
line 15: "P8_15" unused input active-high
line 16: "P9_15A" unused input active-high
line 17: "P9_23" unused input active-high
line 18: "P9_14 [ehrpwm1a]" unused input active-high
line 19: "P9_16 [ehrpwm1b]" unused input active-high
line 20: "[emmc]" unused input active-high
line 21: "[usr0 led]" "beaglebone:green:heartbeat" output active-high [used]
line 22: "[usr1 led]" "beaglebone:green:mmc0" output active-high [used]
line 23: "[usr2 led]" "beaglebone:green:usr2" output active-high [used]
line 24: "[usr3 led]" "beaglebone:green:usr3" output active-high [used]
line 25: "[hdmi]" "interrupt" input active-high [used]
line 26: "[usb]" unused input active-high
line 27: "[hdmi audio]" "enable" output active-high [used]
line 28: "P9_12" unused input active-high
line 29: "P8_26" unused input active-high
line 30: "P8_21 [emmc]" unused input active-high
line 31: "P8_20 [emmc]" unused input active-high
gpiochip2 - 32 lines:
line 0: "P9_15B" unused input active-high
line 1: "P8_18" unused input active-high
line 2: "P8_7" unused input active-high
line 3: "P8_8" unused input active-high
line 4: "P8_10" unused input active-high
line 5: "P8_9" unused input active-high
line 6: "P8_45 [hdmi]" unused input active-high
line 7: "P8_46 [hdmi]" unused input active-high
line 8: "P8_43 [hdmi]" unused input active-high
line 9: "P8_44 [hdmi]" unused input active-high
line 10: "P8_41 [hdmi]" unused input active-high
line 11: "P8_42 [hdmi]" unused input active-high
line 12: "P8_39 [hdmi]" unused input active-high
line 13: "P8_40 [hdmi]" unused input active-high
line 14: "P8_37 [hdmi]" unused input active-high
line 15: "P8_38 [hdmi]" unused input active-high
line 16: "P8_36 [hdmi]" unused input active-high
line 17: "P8_34 [hdmi]" unused input active-high
line 18: "[ethernet]" unused input active-high
line 19: "[ethernet]" unused input active-high
line 20: "[ethernet]" unused input active-high
line 21: "[ethernet]" unused input active-high
line 22: "P8_27 [hdmi]" unused input active-high
line 23: "P8_29 [hdmi]" unused input active-high
line 24: "P8_28 [hdmi]" unused input active-high
line 25: "P8_30 [hdmi]" unused input active-high
line 26: "[emmc]" unused input active-high
line 27: "[emmc]" unused input active-high
line 28: "[emmc]" unused input active-high
line 29: "[emmc]" unused input active-high
line 30: "[emmc]" unused input active-high
line 31: "[emmc]" unused input active-high
gpiochip3 - 32 lines:
line 0: "[ethernet]" unused input active-high
line 1: "[ethernet]" unused input active-high
line 2: "[ethernet]" unused input active-high
line 3: "[ethernet]" unused input active-high
line 4: "[ethernet]" unused input active-high
line 5: "[i2c0]" unused input active-high
line 6: "[i2c0]" unused input active-high
line 7: "[emu]" unused input active-high
line 8: "[emu]" unused input active-high
line 9: "[ethernet]" unused input active-high
line 10: "[ethernet]" unused input active-high
line 11: "[NC]" unused input active-high
line 12: "[NC]" unused input active-high
line 13: "[usb]" unused input active-high
line 14: "P9_31 [spi1_sclk]" unused input active-high
line 15: "P9_29 [spi1_d0]" unused input active-high
line 16: "P9_30 [spi1_d1]" unused input active-high
line 17: "P9_28 [spi1_cs0]" unused input active-high
line 18: "P9_42B [ecappwm0]" unused input active-high
line 19: "P9_27" unused input active-high
line 20: "P9_41A" unused input active-high
line 21: "P9_25" unused input active-high
line 22: "[NC]" unused input active-high
line 23: "[NC]" unused input active-high
line 24: "[NC]" unused input active-high
line 25: "[NC]" unused input active-high
line 26: "[NC]" unused input active-high
line 27: "[NC]" unused input active-high
line 28: "[NC]" unused input active-high
line 29: "[NC]" unused input active-high
line 30: "[NC]" unused input active-high
line 31: "[NC]" unused input active-high
[0] https://git.io/JfgOd
[1] https://beagleboard.org/capes
[1] https://beagleboard.org/Support/bone101
[2] https://beagleboard.org/static/images/cape-headers.png
Reviewed-by: Jason Kridner <jason@beagleboard.org>
Reviewed-by: Robert Nelson <robertcnelson@gmail.com>
Signed-off-by: Drew Fustini <drew@beagleboard.org>
---
V2 change:
- include output of gpioinfo to illustrate how the gpio-line-names
will appear. My apologies for forgetting to include in V1.
V1 note:
I had posted a patch with these line names for am335x-bone-common.dtsi
but Grygorii Strashko pointed out that the names are not applicable to
all BeagleBone models. Thus I have created this patch to add these
names just for the BeagleBone Black.
arch/arm/boot/dts/am335x-boneblack.dts | 144 +++++++++++++++++++++++++
1 file changed, 144 insertions(+)
diff --git a/arch/arm/boot/dts/am335x-boneblack.dts b/arch/arm/boot/dts/am335x-boneblack.dts
index d3928662aed4..5f31e832eb82 100644
--- a/arch/arm/boot/dts/am335x-boneblack.dts
+++ b/arch/arm/boot/dts/am335x-boneblack.dts
@@ -23,3 +23,147 @@ oppnitro-1000000000 {
opp-supported-hw = <0x06 0x0100>;
};
};
+
+&gpio0 {
+ gpio-line-names =
+ "[ethernet]",
+ "[ethernet]",
+ "P9_22 [spi0_sclk]",
+ "P9_21 [spi0_d0]",
+ "P9_18 [spi0_d1]",
+ "P9_17 [spi0_cs0]",
+ "[sd card]",
+ "P9_42A [ecappwm0]",
+ "P8_35 [hdmi]",
+ "P8_33 [hdmi]",
+ "P8_31 [hdmi]",
+ "P8_32 [hdmi]",
+ "P9_20 [i2c2_sda]",
+ "P9_19 [i2c2_scl]",
+ "P9_26 [uart1_rxd]",
+ "P9_24 [uart1_txd]",
+ "[ethernet]",
+ "[ethernet]",
+ "[usb]",
+ "[hdmi]",
+ "P9_41B",
+ "[ethernet]",
+ "P8_19 [ehrpwm2a]",
+ "P8_13 [ehrpwm2b]",
+ "[NC]",
+ "[NC]",
+ "P8_14",
+ "P8_17",
+ "[ethernet]",
+ "[ethernet]",
+ "P9_11 [uart4_rxd]",
+ "P9_13 [uart4_txd]";
+};
+
+&gpio1 {
+ gpio-line-names =
+ "P8_25 [emmc]",
+ "[emmc]",
+ "P8_5 [emmc]",
+ "P8_6 [emmc]",
+ "P8_23 [emmc]",
+ "P8_22 [emmc]",
+ "P8_3 [emmc]",
+ "P8_4 [emmc]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "P8_12",
+ "P8_11",
+ "P8_16",
+ "P8_15",
+ "P9_15A",
+ "P9_23",
+ "P9_14 [ehrpwm1a]",
+ "P9_16 [ehrpwm1b]",
+ "[emmc]",
+ "[usr0 led]",
+ "[usr1 led]",
+ "[usr2 led]",
+ "[usr3 led]",
+ "[hdmi]",
+ "[usb]",
+ "[hdmi audio]",
+ "P9_12",
+ "P8_26",
+ "P8_21 [emmc]",
+ "P8_20 [emmc]";
+};
+
+&gpio2 {
+ gpio-line-names =
+ "P9_15B",
+ "P8_18",
+ "P8_7",
+ "P8_8",
+ "P8_10",
+ "P8_9",
+ "P8_45 [hdmi]",
+ "P8_46 [hdmi]",
+ "P8_43 [hdmi]",
+ "P8_44 [hdmi]",
+ "P8_41 [hdmi]",
+ "P8_42 [hdmi]",
+ "P8_39 [hdmi]",
+ "P8_40 [hdmi]",
+ "P8_37 [hdmi]",
+ "P8_38 [hdmi]",
+ "P8_36 [hdmi]",
+ "P8_34 [hdmi]",
+ "[ethernet]",
+ "[ethernet]",
+ "[ethernet]",
+ "[ethernet]",
+ "P8_27 [hdmi]",
+ "P8_29 [hdmi]",
+ "P8_28 [hdmi]",
+ "P8_30 [hdmi]",
+ "[emmc]",
+ "[emmc]",
+ "[emmc]",
+ "[emmc]",
+ "[emmc]",
+ "[emmc]";
+};
+
+&gpio3 {
+ gpio-line-names =
+ "[ethernet]",
+ "[ethernet]",
+ "[ethernet]",
+ "[ethernet]",
+ "[ethernet]",
+ "[i2c0]",
+ "[i2c0]",
+ "[emu]",
+ "[emu]",
+ "[ethernet]",
+ "[ethernet]",
+ "[NC]",
+ "[NC]",
+ "[usb]",
+ "P9_31 [spi1_sclk]",
+ "P9_29 [spi1_d0]",
+ "P9_30 [spi1_d1]",
+ "P9_28 [spi1_cs0]",
+ "P9_42B [ecappwm0]",
+ "P9_27",
+ "P9_41A",
+ "P9_25",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]";
+};
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v12 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver
From: Wolfram Sang @ 2020-05-21 20:37 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Tali Perry, Ofer Yehielli, Brendan Higgins, avifishman70,
Tomer Maimon, kfting, Patrick Venture, Nancy Yuen, Benjamin Fair,
Rob Herring, linux-arm-kernel, linux-i2c, OpenBMC Maillist,
devicetree, Linux Kernel Mailing List
In-Reply-To: <20200521145347.GO1634618@smile.fi.intel.com>
[-- Attachment #1: Type: text/plain, Size: 1148 bytes --]
> > > I wondered also about DEBUG_FS entries. I can see their value when
> > > developing the driver. But since this is done now, do they really help a
> > > user to debug a difficult case? I am not sure, and then I wonder if we
> > > should have that code in upstream. I am open for discussion, though.
> >
> > The user wanted to have health monitor implemented on top of the driver.
> > The user has 16 channels connected the multiple devices. All are operated
> > using various daemons in the system. Sometimes the slave devices are power down.
> > Therefor the user wanted to track the health status of the devices.
>
> Ah, then there are these options I have in mind (Wolfram, FYI as well!):
> 1) push with debugfs as a temporary solution and convert to devlink health protocol [1];
> 2) drop it and develop devlink_health solution;
> 3) push debugfs and wait if I²C will gain devlink health support
No need for 2). We can push it now and convert it later. That being
said, I wonder if [1] is suitable for this driver? Things like NACKs and
timeouts happen regularly on an I2C bus and are not a state of bad
health.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v12 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver
From: Tali Perry @ 2020-05-21 20:47 UTC (permalink / raw)
To: Wolfram Sang
Cc: Andy Shevchenko, Ofer Yehielli, Brendan Higgins, avifishman70,
Tomer Maimon, kfting, Patrick Venture, Nancy Yuen, Benjamin Fair,
Rob Herring, linux-arm-kernel, linux-i2c, OpenBMC Maillist,
devicetree, Linux Kernel Mailing List
In-Reply-To: <20200521203758.GA20150@ninjato>
On Thu, May 21, 2020 at 11:37 PM Wolfram Sang <wsa@the-dreams.de> wrote:
>
>
> > > > I wondered also about DEBUG_FS entries. I can see their value when
> > > > developing the driver. But since this is done now, do they really help a
> > > > user to debug a difficult case? I am not sure, and then I wonder if we
> > > > should have that code in upstream. I am open for discussion, though.
> > >
> > > The user wanted to have health monitor implemented on top of the driver.
> > > The user has 16 channels connected the multiple devices. All are operated
> > > using various daemons in the system. Sometimes the slave devices are power down.
> > > Therefor the user wanted to track the health status of the devices.
> >
> > Ah, then there are these options I have in mind (Wolfram, FYI as well!):
> > 1) push with debugfs as a temporary solution and convert to devlink health protocol [1];
> > 2) drop it and develop devlink_health solution;
> > 3) push debugfs and wait if I²C will gain devlink health support
>
> No need for 2). We can push it now and convert it later. That being
> said, I wonder if [1] is suitable for this driver? Things like NACKs and
> timeouts happen regularly on an I2C bus and are not a state of bad
> health.
>
Agree, having a timeout every now and then is not an issue. The user
is interested
in cases when the number of timeouts\BER\nack\recovery are high.
^ 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