* Re: [PATCH v11 5/7] overlay: Documentation for the overlay sugar syntax
From: David Gibson @ 2016-11-30 0:39 UTC (permalink / raw)
To: Frank Rowand
Cc: Pantelis Antoniou, Jon Loeliger, Grant Likely, Rob Herring,
Jan Luebbe, Sascha Hauer, Phil Elwell, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <583DB09C.7060105-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2815 bytes --]
On Tue, Nov 29, 2016 at 08:45:16AM -0800, Frank Rowand wrote:
> On 11/28/16 21:10, David Gibson wrote:
> > On Mon, Nov 28, 2016 at 08:36:07PM -0800, Frank Rowand wrote:
> >> On 11/28/16 19:10, David Gibson wrote:
> >>> On Mon, Nov 28, 2016 at 06:05:39PM +0200, Pantelis Antoniou wrote:
> >>>> There exists a syntactic sugar version of overlays which
> >>>> make them simpler to write for the trivial case of a single target.
> >>
> >> It also works for multiple targets. (See the example I provided in
> >> my comment to v10.)
> >>
> >>
> >>>>
> >>>> Document it in the device tree object internals.
> >>>>
> >>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> >>>
> >>> I'm with Frank that I think this, rather than being regarded mere
> >>> syntactic sugar, should be considered the primary way of describing
> >>> overlays.
> >>>
> >>> Obviously we need to support the fully written out version as well.
> >>
> >> If we need to support the fully written out version, can we make that
> >> a discouraged, non-preferred method? Maybe require an option to
> >> enable compiling this style of dts?
> >
> > Yeah. To avoid further proliferation of options, I'm thinking a
> > single "backwards compat" option which would:
> > - Use the dtb magic instead of dtb magic
> > - Disable checks which would reject explicit creation of
> > __overlay__ / __symbols__ / __fixups__ nodes
> > - Anything other special behaviour we need
> >
> >> I can imagine some reasons to support the fully written out version,
> >> but can we document what those reasons are?
> >
> > I believe the main one is the dts files in this format out in the
> > field. Mind you, I guess we're already requiring them to tweak how
> > they declare the /plugin/ option.
>
> It might be easy to write a program that transforms the expanded
> format to the simple format. I'll try to make some time to see
> how difficult it is. The transformation is relatively easy to
> do manually, but I don't know how many dts files would need to
> be converted.
It's not totally trivial, because such a program would basically need
a full dts parser. But.. if we change the dtc internals to work with
a list of overlays rather than a single tree, there's a relatively
obvious path to it: we can implement parsing a dtbo input into a set
of fragments rather than an assembled overlay and dts output in
fragment form. Then converting from old-dts to dtb and back to dts
would pretty much do what's needed. At the cost of losing comments,
though :/.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RESEND PATCH 2/2] PCI: rockchip: Add quirk to disable RC's ASPM L0s
From: Shawn Lin @ 2016-11-30 0:41 UTC (permalink / raw)
To: Rob Herring
Cc: shawn.lin-TNX95d0MmH7DzftRWevZcw, Bjorn Helgaas,
linux-pci-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
open list:ARM/Rockchip SoC..., Wenrui Li, Brian Norris,
Jeffy Chen, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CAL_JsqKkK15Jb3tWh1Mw7RU6d8=6RYMjSEs-c4EQySaUAwnFUA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi Rob,
On 2016/11/29 23:34, Rob Herring wrote:
> On Sun, Nov 13, 2016 at 10:11 PM, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
>> Rockchip's RC outputs 100MHz reference clock but there are
>> two methods for PHY to generate it.
>>
>> (1)One of them is to use system PLL to generate 100MHz clock and
>> the PHY will relock it and filter signal noise then outputs the
>> reference clock.
>>
>> (2)Another way is to share Soc's 24MHZ crystal oscillator with
>> PHY and force PHY's DLL to generate 100MHz internally.
>>
>> When using case(2), the exit from L0s doesn't work fine occasionally
>> due to the broken design of RC receiver's logical circuit. So even if
>> we use extended-synch, it still fails for PHY to relock the bits from
>> FTS sometimes. This will hang the system.
>>
>> Maybe we could argue that why not use case(1) to avoid it? The reason
>> is that as we could see the reference clock is derived from system PLL
>> and the path from it to PHY isn't so clean which means there are some
>> noise introduced by power-domain and other buses can't be filterd out
>> by PHY and we could see noise from the frequency spectrum by oscilloscope.
>> This makes the TX compatibility test a little difficult to pass the spec.
>> So case(1) and case(2) are both used indeed now. If using case(2), we
>> should disable RC's L0s support, and that is why we need this property to
>> indicate this quirk.
>>
>> Also after checking quirk.c, I noticed there is already a quirk for
>> disabling L0s unconditionally, quirk_disable_aspm_l0s. But obviously we
>> shouldn't do that as mentioned above that case(1) could still works fine
>> with L0s.
>>
>> Reported-by: Jeffy Chen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> Cc: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>
>> ---
>>
>> Documentation/devicetree/bindings/pci/rockchip-pcie.txt | 2 ++
>> drivers/pci/host/pcie-rockchip.c | 9 +++++++++
>> 2 files changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>> index ba67b39..cfa44a7 100644
>> --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>> @@ -42,6 +42,8 @@ Required properties:
>> Optional Property:
>> - ep-gpios: contain the entry for pre-reset gpio
>> - num-lanes: number of lanes to use
>> +- quirk,aspm-no-l0s: RC won't support ASPM L0s. This property is needed if
>
> quirk is not a vendor. Drop it.
I guess you want me to drop the "quirk" prefix and just use
aspm-no-l0s instaed?
>
>> + using 24MHz OSC for RC's PHY.
>> - vpcie3v3-supply: The phandle to the 3.3v regulator to use for PCIe.
>> - vpcie1v8-supply: The phandle to the 1.8v regulator to use for PCIe.
>> - vpcie0v9-supply: The phandle to the 0.9v regulator to use for PCIe.
>
>
>
--
Best Regards
Shawn Lin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net-next v3 0/4] Fix OdroidC2 Gigabit Tx link issue
From: Florian Fainelli @ 2016-11-30 0:43 UTC (permalink / raw)
To: David Miller, jbrunet-rdvid1DuHRBWk0Htik3J/w
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA,
carlo-KA+7E9HrN00dnm+yROfE0A, khilman-rdvid1DuHRBWk0Htik3J/w,
peppe.cavallaro-qxv4g6HH51o, alexandre.torgue-qxv4g6HH51o,
martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg,
neolynx-Re5JQEeQqe8AvxtiuMwx3w, andrew-g2DYL2Zd6BY,
narmstrong-rdvid1DuHRBWk0Htik3J/w,
linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161129.193853.827524417068912706.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
On 11/29/2016 04:38 PM, David Miller wrote:
> From: Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> Date: Mon, 28 Nov 2016 10:46:45 +0100
>
>> This patchset fixes an issue with the OdroidC2 board (DWMAC + RTL8211F).
>> The platform seems to enter LPI on the Rx path too often while performing
>> relatively high TX transfer. This eventually break the link (both Tx and
>> Rx), and require to bring the interface down and up again to get the Rx
>> path working again.
>>
>> The root cause of this issue is not fully understood yet but disabling EEE
>> advertisement on the PHY prevent this feature to be negotiated.
>> With this change, the link is stable and reliable, with the expected
>> throughput performance.
>>
>> The patchset adds options in the generic phy driver to disable EEE
>> advertisement, through device tree. The way it is done is very similar
>> to the handling of the max-speed property.
>
> Patches 1-3 applied to net-next, thanks.
Meh, there was a v4 submitted shortly after, and I objected to the whole
idea of using that kind of Device Tree properties to disable EEE, we can
send reverts though..
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 1/2] mtd: spi-nor: Bindings for Rockchip serial flash controller
From: Shawn Lin @ 2016-11-30 0:55 UTC (permalink / raw)
To: Marek Vasut, David Woodhouse, Brian Norris
Cc: shawn.lin-TNX95d0MmH7DzftRWevZcw, Cyrille Pitchen, Rob Herring,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Heiko Stuebner
In-Reply-To: <7fd2cdc0-2103-d254-c7b2-d2552a32a738-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On 2016/11/25 21:30, Marek Vasut wrote:
> On 11/18/2016 03:59 AM, Shawn Lin wrote:
>> Add binding document for the Rockchip serial flash controller.
>>
>> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> ---
>>
>> Changes in v2: None
>>
>> .../devicetree/bindings/mtd/rockchip-sfc.txt | 31 ++++++++++++++++++++++
>> 1 file changed, 31 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/mtd/rockchip-sfc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/rockchip-sfc.txt b/Documentation/devicetree/bindings/mtd/rockchip-sfc.txt
>> new file mode 100644
>> index 0000000..28430ce
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mtd/rockchip-sfc.txt
>> @@ -0,0 +1,31 @@
>> +Rockchip Serial Flash Controller
>> +
>> +Required properties:
>> +- compatible : Should be
>> + "rockchip,rk1108-sfc", "rockchip,sfc" for ROCKCHIP RK1108.
>> +- address-cells : Should be 1.
>> +- size-cells : Should be 0.
>
> Shouldn't these two props have a # prefix ? I'm not sure they should
> even be part of this binding document at all.
>
>> +- clocks: Must contain two entries for each entry in clock-names.
>> +- clock-names: Shall be "sfc" for the transfer-clock, and "hsfc" for
>> + the peripheral clock.
>> +- interrupts : Should contain the interrupt for the device.
>> +- reg: Physical base address of the controller and length of memory mapped.
>> +
>> +Optional properties:
>> +- rockchip,sfc-no-dma: Indicate the controller doesn't support dma transfer.
>
> DMA should be in capital letters.
Both of DMA or dma could be found by grepping the
Documentation/devicetree/bindings/, so I think it
isn't a big deal for this. :)
>
>> +Example:
>> +nor_flash: sfc@301c0000 {
>> + compatible = "rockchip,rk1108-sfc", "rockchip,sfc";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + clocks = <&cru SCLK_SFC>, <&cru HCLK_SFC>;
>> + clock-names = "sfc", "hsfc";
>> + interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
>> + reg = <0x301c0000 0x1000>;
>> + spi-nor@0 {
>> + compatible = "jedec,spi-nor";
>> + spi-max-frequency = <12000000>;
>> + reg = <0>;
>> + };
>> +};
>>
>
>
--
Best Regards
Shawn Lin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/2] mtd: spi-nor: add rockchip serial flash controller driver
From: Shawn Lin @ 2016-11-30 0:57 UTC (permalink / raw)
To: Marek Vasut, Rob Herring, David Woodhouse, Brian Norris
Cc: shawn.lin-TNX95d0MmH7DzftRWevZcw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Heiko Stuebner,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <b513a489-5913-ffe8-69f1-7201943a05a3-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
在 2016/11/25 21:54, Marek Vasut 写道:
> On 11/21/2016 03:51 AM, Shawn Lin wrote:
>> Hi Marek,
>
> Hi!
>
> [...]
>
>>>> sfc->num_chip stands for how many flashes registered successfully.
>>>
>>> Does it work if you have a hole in there ? Like if you have a flash on
>>> chipselect 0 and chipselect 2 ?
>>
>> Yes it does, as it won't leave a room for chipselect 1 whose node isn't
>> present, which means there isn't a hole in there at all. :)
>
> Ah , because you are not indexing those NORs with their CS number , right ?
yes. :)
>
>
--
Best Regards
Shawn Lin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 2/2] mtd: spi-nor: add rockchip serial flash controller driver
From: Shawn Lin @ 2016-11-30 0:58 UTC (permalink / raw)
To: Marek Vasut, David Woodhouse, Brian Norris
Cc: shawn.lin-TNX95d0MmH7DzftRWevZcw, Cyrille Pitchen, Rob Herring,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Heiko Stuebner
In-Reply-To: <020fe898-8476-42cd-9591-d2bc97263457-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
在 2016/11/25 21:55, Marek Vasut 写道:
> On 11/18/2016 03:59 AM, Shawn Lin wrote:
>> Add rockchip serial flash controller driver
>>
>> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>
> [...]
>
>> +static void rockchip_sfc_unregister_all(struct rockchip_sfc *sfc)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < sfc->num_chip; i++)
>> + mtd_device_unregister(&(sfc->flash[sfc->num_chip].nor.mtd));
> ^^^^^^^^^^^^^
> This will always unregister the same flash, no ? This cannot work.
Ah, yup, I will fix this. :)
>
>> +}
>
> [...]
>
--
Best Regards
Shawn Lin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 1/2] i2c: aspeed: added driver for Aspeed I2C
From: Brendan Higgins @ 2016-11-30 0:59 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: mark.rutland, Wolfram Sang, robh+dt, linux-i2c, devicetree,
Joel Stanley, OpenBMC Maillist
In-Reply-To: <00867f8e-e861-4184-6179-25e996789762@mleia.com>
Thanks for the comments!
Most of the stuff you suggested was pretty straightforward and will be
in the v5 patchset which I will send out in a bit.
I removed my implementation of the irq_chip; it appears that there is already a
dummy_irq_chip that is intended for just this kind of thing. Let me
know if you think
we still need to loop in the irqchip people.
The remaining comments are addressed below:
> > +#define ASPEED_I2CD_MULTI_MASTER_DIS BIT(15)
>
> Unused macro, please remove.
No, it is used below:
> + /* Enable Master Mode */
> + aspeed_i2c_write(bus, aspeed_i2c_read(bus, ASPEED_I2C_FUN_CTRL_REG) |
> + ASPEED_I2CD_MASTER_EN |
> + ASPEED_I2CD_MULTI_MASTER_DIS, ASPEED_I2C_FUN_CTRL_REG);
> > +#define ASPEED_I2CD_SDA_DRIVE_1T_EN BIT(8)
>
> Unused macro, please remove.
No, it is used below:
> + /* Set AC Timing */
> + if (clk_freq / 1000 > 400) {
> + aspeed_i2c_write(bus, aspeed_i2c_read(bus,
> + ASPEED_I2C_FUN_CTRL_REG) |
> + ASPEED_I2CD_M_HIGH_SPEED_EN |
> + ASPEED_I2CD_M_SDA_DRIVE_1T_EN |
> + ASPEED_I2CD_SDA_DRIVE_1T_EN,
> + ASPEED_I2C_FUN_CTRL_REG);
> > +#define ASPEED_I2CD_SLAVE_EN BIT(1)
>
> Unused macro, please remove. You add slave support, may be you
> need this control, but it is unused.
No, it is used below:
> + /* Switch from master mode to slave mode. */
> + func_ctrl_reg_val = aspeed_i2c_read(bus, ASPEED_I2C_FUN_CTRL_REG);
> + func_ctrl_reg_val &= ~ASPEED_I2CD_MASTER_EN;
> + func_ctrl_reg_val |= ASPEED_I2CD_SLAVE_EN;
> > + /* Slave was asked to stop. */
> > + if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
> > + status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
> > + bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> > + }
>
> Add an empty line here.
>
> > + if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
> > + status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> > + bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> > + }
Actually, I think this makes more sense without a space as these are
both stop conditions.
> > +static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
> > + struct platform_device *pdev)
> > +{
> > + struct clk *pclk;
> > + u32 clk_freq;
> > + u32 divider_ratio;
> > + int ret;
> > +
> > + pclk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(pclk)) {
> > + dev_err(&pdev->dev, "clk_get failed\n");
> > + return PTR_ERR(pclk);
> > + }
> > + ret = of_property_read_u32(pdev->dev.of_node,
> > + "clock-frequency", &clk_freq);
> > + if (ret < 0) {
> > + dev_err(&pdev->dev,
> > + "Could not read clock-frequency property\n");
> > + clk_freq = 100000;
> > + }
> > + divider_ratio = clk_get_rate(pclk) / clk_freq;
> > + /* We just need the clock rate, we don't actually use the clk object. */
> > + devm_clk_put(&pdev->dev, pclk);
>
> Does the controller have a clock supply? If yes, shall the clock be
> enabled before issuing command to the controller?
No, the clock source for the busses is the APB's clock. The chip does
not really expose
any sophisticated way to manipulate it, so we can just assume it is
always on and is fixed
frequency. Additionally, each bus has its own clock which is just a
divider on the APB's
clock which we set up here. The controller does not participate in this.
> > +static int aspeed_i2c_probe_bus(struct platform_device *pdev)
> > +{
> > + struct aspeed_i2c_bus *bus;
> > + struct aspeed_i2c_controller *controller =
> > + dev_get_drvdata(pdev->dev.parent);
>
> How do you ensure that "controller" device _driver_ is initialized
> at this point? This is a critical race condition.
aspeed_i2c_probe_controller(...) is responsible for creating the bus
nodes right before it
finishes, so it makes sure that it is sufficiently initialized before
allowing its child busses
to be probed:
> + for_each_child_of_node(pdev->dev.of_node, np) {
> + of_platform_device_create(np, NULL, &pdev->dev);
> + of_node_put(np);
> + }
Cheers
^ permalink raw reply
* [PATCH v5 0/2] i2c: aspeed: added driver for Aspeed I2C
From: Brendan Higgins @ 2016-11-30 1:00 UTC (permalink / raw)
To: wsa-z923LK4zBo2bacvFa/9K2g, vz-ChpfBGZJDbMAvxtiuMwx3w,
clg-Bxea+6Xhats, mouse-Pma6HLj0uuo,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, joel-U3u1mxZcP9KHXe+LvDLADg,
openbmc-uLR06cmDAlY/bJ5BZ2RsiQ
Addressed comments from:
- Cedric in: https://www.spinics.net/lists/devicetree/msg151685.html
- Kachalov in: https://www.spinics.net/lists/devicetree/msg151819.html
- Vladimir in: https://www.spinics.net/lists/devicetree/msg152454.html
Changes since previous update:
- Removed irq_chip used to multiplex IRQ and replaced it with dummy_irq_chip
along with some other IRQ cleanup.
- Addressed comments from Cedric, and Vladimir, mostly stylistic things and
using devm managed resources.
- Increased max clock frequency before the bus is put in HighSpeed mode, as
per Kachalov's comment.
Changes have been tested on the Aspeed evaluation board as before.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v5 1/2] i2c: aspeed: added driver for Aspeed I2C
From: Brendan Higgins @ 2016-11-30 1:00 UTC (permalink / raw)
To: wsa, vz, clg, mouse, robh+dt, mark.rutland
Cc: linux-i2c, devicetree, joel, openbmc, Brendan Higgins
In-Reply-To: <1480467618-7497-1-git-send-email-brendanhiggins@google.com>
Added initial master and slave support for Aspeed I2C controller.
Supports fourteen busses present in ast24xx and ast25xx BMC SoCs by
Aspeed.
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
Changes for v2:
- Added single module_init (multiple was breaking some builds).
Changes for v3:
- Removed "bus" device tree param; now extracted from bus address offset
Changes for v4:
- I2C adapter number is now generated dynamically unless specified in alias.
Changes for v5:
- Removed irq_chip used to multiplex IRQ and replaced it with dummy_irq_chip
along with some other IRQ cleanup.
- Addressed comments from Cedric, and Vladimir, mostly stylistic things and
using devm managed resources.
- Increased max clock frequency before the bus is put in HighSpeed mode, as
per Kachalov's comment.
---
drivers/i2c/busses/Kconfig | 10 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-aspeed.c | 839 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 850 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-aspeed.c
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index d252276..e8cf750 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -325,6 +325,16 @@ config I2C_POWERMAC
comment "I2C system bus drivers (mostly embedded / system-on-chip)"
+config I2C_ASPEED
+ tristate "Aspeed AST2xxx SoC I2C Controller"
+ depends on ARCH_ASPEED
+ help
+ If you say yes to this option, support will be included for the
+ Aspeed AST2xxx SoC I2C controller.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-aspeed.
+
config I2C_AT91
tristate "Atmel AT91 I2C Two-Wire interface (TWI)"
depends on ARCH_AT91
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 29764cc..73fec22 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_I2C_HYDRA) += i2c-hydra.o
obj-$(CONFIG_I2C_POWERMAC) += i2c-powermac.o
# Embedded system I2C/SMBus host controller drivers
+obj-$(CONFIG_I2C_ASPEED) += i2c-aspeed.o
obj-$(CONFIG_I2C_AT91) += i2c-at91.o
obj-$(CONFIG_I2C_AU1550) += i2c-au1550.o
obj-$(CONFIG_I2C_AXXIA) += i2c-axxia.o
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
new file mode 100644
index 0000000..0e68808
--- /dev/null
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -0,0 +1,839 @@
+/*
+ * I2C adapter for the ASPEED I2C bus.
+ *
+ * Copyright (C) 2012-2016 ASPEED Technology Inc.
+ * Copyright 2016 IBM Corporation
+ * Copyright 2016 Google, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+/* I2C Register */
+#define ASPEED_I2C_FUN_CTRL_REG 0x00
+#define ASPEED_I2C_AC_TIMING_REG1 0x04
+#define ASPEED_I2C_AC_TIMING_REG2 0x08
+#define ASPEED_I2C_INTR_CTRL_REG 0x0c
+#define ASPEED_I2C_INTR_STS_REG 0x10
+#define ASPEED_I2C_CMD_REG 0x14
+#define ASPEED_I2C_DEV_ADDR_REG 0x18
+#define ASPEED_I2C_BYTE_BUF_REG 0x20
+
+#define ASPEED_I2C_NUM_BUS 14
+
+/* Global Register Definition */
+/* 0x00 : I2C Interrupt Status Register */
+/* 0x08 : I2C Interrupt Target Assignment */
+
+/* Device Register Definition */
+/* 0x00 : I2CD Function Control Register */
+#define ASPEED_I2CD_MULTI_MASTER_DIS BIT(15)
+#define ASPEED_I2CD_SDA_DRIVE_1T_EN BIT(8)
+#define ASPEED_I2CD_M_SDA_DRIVE_1T_EN BIT(7)
+#define ASPEED_I2CD_M_HIGH_SPEED_EN BIT(6)
+#define ASPEED_I2CD_SLAVE_EN BIT(1)
+#define ASPEED_I2CD_MASTER_EN BIT(0)
+
+/* 0x08 : I2CD Clock and AC Timing Control Register #2 */
+#define ASPEED_NO_TIMEOUT_CTRL 0
+
+
+/* 0x0c : I2CD Interrupt Control Register &
+ * 0x10 : I2CD Interrupt Status Register
+ *
+ * These share bit definitions, so use the same values for the enable &
+ * status bits.
+ */
+#define ASPEED_I2CD_INTR_SDA_DL_TIMEOUT BIT(14)
+#define ASPEED_I2CD_INTR_BUS_RECOVER_DONE BIT(13)
+#define ASPEED_I2CD_INTR_SLAVE_MATCH BIT(7)
+#define ASPEED_I2CD_INTR_SCL_TIMEOUT BIT(6)
+#define ASPEED_I2CD_INTR_ABNORMAL BIT(5)
+#define ASPEED_I2CD_INTR_NORMAL_STOP BIT(4)
+#define ASPEED_I2CD_INTR_ARBIT_LOSS BIT(3)
+#define ASPEED_I2CD_INTR_RX_DONE BIT(2)
+#define ASPEED_I2CD_INTR_TX_NAK BIT(1)
+#define ASPEED_I2CD_INTR_TX_ACK BIT(0)
+#define ASPEED_I2CD_INTR_ERROR \
+ (ASPEED_I2CD_INTR_ARBIT_LOSS | \
+ ASPEED_I2CD_INTR_ABNORMAL | \
+ ASPEED_I2CD_INTR_SCL_TIMEOUT | \
+ ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
+ ASPEED_I2CD_INTR_TX_NAK)
+#define ASPEED_I2CD_INTR_ALL \
+ (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
+ ASPEED_I2CD_INTR_BUS_RECOVER_DONE | \
+ ASPEED_I2CD_INTR_SCL_TIMEOUT | \
+ ASPEED_I2CD_INTR_ABNORMAL | \
+ ASPEED_I2CD_INTR_NORMAL_STOP | \
+ ASPEED_I2CD_INTR_ARBIT_LOSS | \
+ ASPEED_I2CD_INTR_RX_DONE | \
+ ASPEED_I2CD_INTR_TX_NAK | \
+ ASPEED_I2CD_INTR_TX_ACK)
+
+/* 0x14 : I2CD Command/Status Register */
+#define ASPEED_I2CD_SCL_LINE_STS BIT(18)
+#define ASPEED_I2CD_SDA_LINE_STS BIT(17)
+#define ASPEED_I2CD_BUS_BUSY_STS BIT(16)
+#define ASPEED_I2CD_BUS_RECOVER_CMD BIT(11)
+
+/* Command Bit */
+#define ASPEED_I2CD_M_STOP_CMD BIT(5)
+#define ASPEED_I2CD_M_S_RX_CMD_LAST BIT(4)
+#define ASPEED_I2CD_M_RX_CMD BIT(3)
+#define ASPEED_I2CD_S_TX_CMD BIT(2)
+#define ASPEED_I2CD_M_TX_CMD BIT(1)
+#define ASPEED_I2CD_M_START_CMD BIT(0)
+
+/* 0x18 : I2CD Slave Device Address Register */
+#define ASPEED_I2CD_DEV_ADDR_MASK GENMASK(6, 0)
+
+enum aspeed_i2c_slave_state {
+ ASPEED_I2C_SLAVE_START,
+ ASPEED_I2C_SLAVE_READ_REQUESTED,
+ ASPEED_I2C_SLAVE_READ_PROCESSED,
+ ASPEED_I2C_SLAVE_WRITE_REQUESTED,
+ ASPEED_I2C_SLAVE_WRITE_RECEIVED,
+ ASPEED_I2C_SLAVE_STOP,
+};
+
+struct aspeed_i2c_bus {
+ struct i2c_adapter adap;
+ struct device *dev;
+ void __iomem *base;
+ spinlock_t lock;
+ struct completion cmd_complete;
+ int irq;
+ /* Transaction state. */
+ struct i2c_msg *msg;
+ int msg_pos;
+ u32 cmd_err;
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+ struct i2c_client *slave;
+ enum aspeed_i2c_slave_state slave_state;
+#endif
+};
+
+struct aspeed_i2c_controller {
+ struct device *dev;
+ void __iomem *base;
+ int irq;
+ struct irq_domain *irq_domain;
+};
+
+static inline void aspeed_i2c_write(struct aspeed_i2c_bus *bus, u32 val,
+ u32 reg)
+{
+ writel(val, bus->base + reg);
+}
+
+static inline u32 aspeed_i2c_read(struct aspeed_i2c_bus *bus, u32 reg)
+{
+ return readl(bus->base + reg);
+}
+
+static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
+{
+ unsigned long time_left, flags;
+ int ret = 0;
+ u32 command;
+
+ spin_lock_irqsave(&bus->lock, flags);
+ command = aspeed_i2c_read(bus, ASPEED_I2C_CMD_REG);
+
+ if (command & ASPEED_I2CD_SDA_LINE_STS) {
+ /* Bus is idle: no recovery needed. */
+ if (command & ASPEED_I2CD_SCL_LINE_STS)
+ goto out;
+ dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n",
+ command);
+
+ aspeed_i2c_write(bus, ASPEED_I2CD_M_STOP_CMD,
+ ASPEED_I2C_CMD_REG);
+ reinit_completion(&bus->cmd_complete);
+ spin_unlock_irqrestore(&bus->lock, flags);
+
+ time_left = wait_for_completion_timeout(
+ &bus->cmd_complete, bus->adap.timeout);
+
+ spin_lock_irqsave(&bus->lock, flags);
+ if (time_left == 0)
+ ret = -ETIMEDOUT;
+ else if (bus->cmd_err)
+ ret = -EIO;
+ /* Bus error. */
+ } else {
+ dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n",
+ command);
+
+ aspeed_i2c_write(bus, ASPEED_I2CD_BUS_RECOVER_CMD,
+ ASPEED_I2C_CMD_REG);
+ reinit_completion(&bus->cmd_complete);
+ spin_unlock_irqrestore(&bus->lock, flags);
+
+ time_left = wait_for_completion_timeout(
+ &bus->cmd_complete, bus->adap.timeout);
+
+ spin_lock_irqsave(&bus->lock, flags);
+ if (time_left == 0)
+ ret = -ETIMEDOUT;
+ else if (bus->cmd_err)
+ ret = -EIO;
+ /* Recovery failed. */
+ else if (!(aspeed_i2c_read(bus, ASPEED_I2C_CMD_REG) &
+ ASPEED_I2CD_SDA_LINE_STS))
+ ret = -EIO;
+ }
+
+out:
+ spin_unlock_irqrestore(&bus->lock, flags);
+
+ return ret;
+}
+
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
+{
+ u32 command, irq_status, status_ack = 0;
+ struct i2c_client *slave = bus->slave;
+ bool irq_handled = true;
+ u8 value;
+
+ spin_lock(&bus->lock);
+ if (!slave) {
+ irq_handled = false;
+ goto out;
+ }
+
+ command = aspeed_i2c_read(bus, ASPEED_I2C_CMD_REG);
+ irq_status = aspeed_i2c_read(bus, ASPEED_I2C_INTR_STS_REG);
+
+ /* Slave was requested, restart state machine. */
+ if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
+ status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
+ bus->slave_state = ASPEED_I2C_SLAVE_START;
+ }
+
+ /* Slave is not currently active, irq was for someone else. */
+ if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
+ irq_handled = false;
+ goto out;
+ }
+
+ dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
+ irq_status, command);
+
+ /* Slave was sent something. */
+ if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
+ value = aspeed_i2c_read(bus, ASPEED_I2C_BYTE_BUF_REG) >> 8;
+ /* Handle address frame. */
+ if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
+ if (value & 0x1)
+ bus->slave_state =
+ ASPEED_I2C_SLAVE_READ_REQUESTED;
+ else
+ bus->slave_state =
+ ASPEED_I2C_SLAVE_WRITE_REQUESTED;
+ }
+ status_ack |= ASPEED_I2CD_INTR_RX_DONE;
+ }
+
+ /* Slave was asked to stop. */
+ if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
+ status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
+ bus->slave_state = ASPEED_I2C_SLAVE_STOP;
+ }
+ if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
+ status_ack |= ASPEED_I2CD_INTR_TX_NAK;
+ bus->slave_state = ASPEED_I2C_SLAVE_STOP;
+ }
+
+ switch (bus->slave_state) {
+ case ASPEED_I2C_SLAVE_READ_REQUESTED:
+ if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
+ dev_err(bus->dev, "Unexpected ACK on read request.\n");
+ bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
+
+ i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
+ aspeed_i2c_write(bus, value, ASPEED_I2C_BYTE_BUF_REG);
+ aspeed_i2c_write(bus, ASPEED_I2CD_S_TX_CMD, ASPEED_I2C_CMD_REG);
+ break;
+ case ASPEED_I2C_SLAVE_READ_PROCESSED:
+ status_ack |= ASPEED_I2CD_INTR_TX_ACK;
+ if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
+ dev_err(bus->dev,
+ "Expected ACK after processed read.\n");
+ i2c_slave_event(slave, I2C_SLAVE_READ_PROCESSED, &value);
+ aspeed_i2c_write(bus, value, ASPEED_I2C_BYTE_BUF_REG);
+ aspeed_i2c_write(bus, ASPEED_I2CD_S_TX_CMD, ASPEED_I2C_CMD_REG);
+ break;
+ case ASPEED_I2C_SLAVE_WRITE_REQUESTED:
+ bus->slave_state = ASPEED_I2C_SLAVE_WRITE_RECEIVED;
+ i2c_slave_event(slave, I2C_SLAVE_WRITE_REQUESTED, &value);
+ break;
+ case ASPEED_I2C_SLAVE_WRITE_RECEIVED:
+ i2c_slave_event(slave, I2C_SLAVE_WRITE_RECEIVED, &value);
+ break;
+ case ASPEED_I2C_SLAVE_STOP:
+ i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
+ break;
+ default:
+ dev_err(bus->dev, "unhandled slave_state: %d\n",
+ bus->slave_state);
+ break;
+ }
+
+ if (status_ack != irq_status)
+ dev_err(bus->dev,
+ "irq handled != irq. expected %x, but was %x\n",
+ irq_status, status_ack);
+ aspeed_i2c_write(bus, status_ack, ASPEED_I2C_INTR_STS_REG);
+
+out:
+ spin_unlock(&bus->lock);
+ return irq_handled;
+}
+#endif
+
+static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
+{
+ u32 irq_status;
+
+ spin_lock(&bus->lock);
+ irq_status = aspeed_i2c_read(bus, ASPEED_I2C_INTR_STS_REG);
+ bus->cmd_err = irq_status & ASPEED_I2CD_INTR_ERROR;
+
+ dev_dbg(bus->dev, "master irq status 0x%08x\n", irq_status);
+
+ /* No message to transfer. */
+ if (bus->cmd_err ||
+ (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) ||
+ (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE)) {
+ complete(&bus->cmd_complete);
+ goto out;
+ } else if (!bus->msg || bus->msg_pos >= bus->msg->len)
+ goto out;
+
+ if ((bus->msg->flags & I2C_M_RD) &&
+ (irq_status & ASPEED_I2CD_INTR_RX_DONE)) {
+ bus->msg->buf[bus->msg_pos++] = aspeed_i2c_read(
+ bus, ASPEED_I2C_BYTE_BUF_REG) >> 8;
+ if (bus->msg_pos + 1 < bus->msg->len)
+ aspeed_i2c_write(bus, ASPEED_I2CD_M_RX_CMD,
+ ASPEED_I2C_CMD_REG);
+ else if (bus->msg_pos < bus->msg->len)
+ aspeed_i2c_write(bus, ASPEED_I2CD_M_RX_CMD |
+ ASPEED_I2CD_M_S_RX_CMD_LAST,
+ ASPEED_I2C_CMD_REG);
+ } else if (!(bus->msg->flags & I2C_M_RD) &&
+ (irq_status & ASPEED_I2CD_INTR_TX_ACK)) {
+ aspeed_i2c_write(bus, bus->msg->buf[bus->msg_pos++],
+ ASPEED_I2C_BYTE_BUF_REG);
+ aspeed_i2c_write(bus, ASPEED_I2CD_M_TX_CMD, ASPEED_I2C_CMD_REG);
+ }
+
+ /* Transmission complete: notify caller. */
+ if (bus->msg_pos >= bus->msg->len)
+ complete(&bus->cmd_complete);
+out:
+ aspeed_i2c_write(bus, irq_status, ASPEED_I2C_INTR_STS_REG);
+ spin_unlock(&bus->lock);
+
+ return true;
+}
+
+static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
+{
+ struct aspeed_i2c_bus *bus = dev_id;
+
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+ if (aspeed_i2c_slave_irq(bus)) {
+ dev_dbg(bus->dev, "irq handled by slave.\n");
+ return IRQ_HANDLED;
+ }
+#endif
+
+ if (aspeed_i2c_master_irq(bus)) {
+ dev_dbg(bus->dev, "irq handled by master.\n");
+ return IRQ_HANDLED;
+ }
+
+ dev_err(bus->dev, "irq not handled properly!\n");
+ return IRQ_NONE;
+}
+
+static int aspeed_i2c_master_single_xfer(struct i2c_adapter *adap,
+ struct i2c_msg *msg)
+{
+ u32 command = ASPEED_I2CD_M_START_CMD | ASPEED_I2CD_M_TX_CMD;
+ struct aspeed_i2c_bus *bus = adap->algo_data;
+ unsigned long time_left, flags;
+ int ret = msg->len;
+ u8 slave_addr;
+
+ spin_lock_irqsave(&bus->lock, flags);
+ bus->msg = msg;
+ bus->msg_pos = 0;
+ slave_addr = msg->addr << 1;
+
+ if (msg->flags & I2C_M_RD) {
+ slave_addr |= 1;
+ command |= ASPEED_I2CD_M_RX_CMD;
+ if (msg->len == 1)
+ command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
+ }
+
+ aspeed_i2c_write(bus, slave_addr, ASPEED_I2C_BYTE_BUF_REG);
+ aspeed_i2c_write(bus, command, ASPEED_I2C_CMD_REG);
+ reinit_completion(&bus->cmd_complete);
+ spin_unlock_irqrestore(&bus->lock, flags);
+
+ time_left = wait_for_completion_timeout(
+ &bus->cmd_complete, bus->adap.timeout);
+ if (time_left == 0)
+ return -ETIMEDOUT;
+
+ spin_lock_irqsave(&bus->lock, flags);
+ if (bus->cmd_err)
+ ret = -EIO;
+
+ bus->msg = NULL;
+ spin_unlock_irqrestore(&bus->lock, flags);
+
+ return ret;
+}
+
+static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
+ struct i2c_msg *msgs, int num)
+{
+ struct aspeed_i2c_bus *bus = adap->algo_data;
+ unsigned long time_left, flags;
+ int ret, i;
+
+ /* If bus is busy, attempt recovery. We assume a single master
+ * environment.
+ */
+ if (aspeed_i2c_read(bus, ASPEED_I2C_CMD_REG) &
+ ASPEED_I2CD_BUS_BUSY_STS) {
+ ret = aspeed_i2c_recover_bus(bus);
+ if (ret)
+ return ret;
+ }
+
+ for (i = 0; i < num; i++) {
+ ret = aspeed_i2c_master_single_xfer(adap, &msgs[i]);
+ if (ret < 0)
+ break;
+ /* TODO: Support other forms of I2C protocol mangling. */
+ if (msgs[i].flags & I2C_M_STOP) {
+ spin_lock_irqsave(&bus->lock, flags);
+ aspeed_i2c_write(bus, ASPEED_I2CD_M_STOP_CMD,
+ ASPEED_I2C_CMD_REG);
+ reinit_completion(&bus->cmd_complete);
+ spin_unlock_irqrestore(&bus->lock, flags);
+
+ time_left = wait_for_completion_timeout(
+ &bus->cmd_complete, bus->adap.timeout);
+ if (time_left == 0)
+ return -ETIMEDOUT;
+ }
+ }
+
+ spin_lock_irqsave(&bus->lock, flags);
+ aspeed_i2c_write(bus, ASPEED_I2CD_M_STOP_CMD, ASPEED_I2C_CMD_REG);
+ reinit_completion(&bus->cmd_complete);
+ spin_unlock_irqrestore(&bus->lock, flags);
+
+ time_left = wait_for_completion_timeout(
+ &bus->cmd_complete, bus->adap.timeout);
+ if (time_left == 0)
+ return -ETIMEDOUT;
+
+ /* If nothing went wrong, return number of messages transferred. */
+ if (ret < 0)
+ return ret;
+ else
+ return i;
+}
+
+static u32 aspeed_i2c_functionality(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_BLOCK_DATA;
+}
+
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+static int aspeed_i2c_reg_slave(struct i2c_client *client)
+{
+ u32 addr_reg_val, func_ctrl_reg_val;
+ struct aspeed_i2c_bus *bus;
+ unsigned long flags;
+
+ bus = client->adapter->algo_data;
+ spin_lock_irqsave(&bus->lock, flags);
+ if (bus->slave) {
+ spin_unlock_irqrestore(&bus->lock, flags);
+ return -EINVAL;
+ }
+
+ /* Set slave addr. */
+ addr_reg_val = aspeed_i2c_read(bus, ASPEED_I2C_DEV_ADDR_REG);
+ addr_reg_val &= ~ASPEED_I2CD_DEV_ADDR_MASK;
+ addr_reg_val |= client->addr & ASPEED_I2CD_DEV_ADDR_MASK;
+ aspeed_i2c_write(bus, addr_reg_val, ASPEED_I2C_DEV_ADDR_REG);
+
+ /* Switch from master mode to slave mode. */
+ func_ctrl_reg_val = aspeed_i2c_read(bus, ASPEED_I2C_FUN_CTRL_REG);
+ func_ctrl_reg_val &= ~ASPEED_I2CD_MASTER_EN;
+ func_ctrl_reg_val |= ASPEED_I2CD_SLAVE_EN;
+ aspeed_i2c_write(bus, func_ctrl_reg_val, ASPEED_I2C_FUN_CTRL_REG);
+
+ bus->slave = client;
+ bus->slave_state = ASPEED_I2C_SLAVE_STOP;
+ spin_unlock_irqrestore(&bus->lock, flags);
+
+ return 0;
+}
+
+static int aspeed_i2c_unreg_slave(struct i2c_client *client)
+{
+ struct aspeed_i2c_bus *bus = client->adapter->algo_data;
+ u32 func_ctrl_reg_val;
+ unsigned long flags;
+
+ spin_lock_irqsave(&bus->lock, flags);
+ if (!bus->slave) {
+ spin_unlock_irqrestore(&bus->lock, flags);
+ return -EINVAL;
+ }
+
+ /* Switch from slave mode to master mode. */
+ func_ctrl_reg_val = aspeed_i2c_read(bus, ASPEED_I2C_FUN_CTRL_REG);
+ func_ctrl_reg_val &= ~ASPEED_I2CD_SLAVE_EN;
+ func_ctrl_reg_val |= ASPEED_I2CD_MASTER_EN;
+ aspeed_i2c_write(bus, func_ctrl_reg_val, ASPEED_I2C_FUN_CTRL_REG);
+
+ bus->slave = NULL;
+ spin_unlock_irqrestore(&bus->lock, flags);
+
+ return 0;
+}
+#endif
+
+static const struct i2c_algorithm aspeed_i2c_algo = {
+ .master_xfer = aspeed_i2c_master_xfer,
+ .functionality = aspeed_i2c_functionality,
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+ .reg_slave = aspeed_i2c_reg_slave,
+ .unreg_slave = aspeed_i2c_unreg_slave,
+#endif
+};
+
+static u32 aspeed_i2c_get_clk_reg_val(u32 divider_ratio)
+{
+ u32 scl_low, scl_high, data;
+ unsigned int inc = 0, div;
+
+ for (div = 0; divider_ratio >= 16; div++) {
+ inc |= (divider_ratio & 1);
+ divider_ratio >>= 1;
+ }
+
+ divider_ratio += inc;
+ scl_low = (divider_ratio >> 1) - 1;
+ scl_high = divider_ratio - scl_low - 2;
+ data = 0x77700300 | (scl_high << 16) | (scl_low << 12) | div;
+ return data;
+}
+
+static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
+ struct platform_device *pdev)
+{
+ u32 clk_freq, divider_ratio;
+ struct clk *pclk;
+ int ret;
+
+ pclk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(pclk)) {
+ dev_err(&pdev->dev, "clk_get failed\n");
+ return PTR_ERR(pclk);
+ }
+ ret = of_property_read_u32(pdev->dev.of_node,
+ "clock-frequency", &clk_freq);
+ if (ret < 0) {
+ dev_err(&pdev->dev,
+ "Could not read clock-frequency property\n");
+ clk_freq = 100000;
+ }
+ divider_ratio = clk_get_rate(pclk) / clk_freq;
+ /* We just need the clock rate, we don't actually use the clk object. */
+ devm_clk_put(&pdev->dev, pclk);
+
+ /* Set AC Timing */
+ if (clk_freq / 1000 > 1000) {
+ aspeed_i2c_write(bus, aspeed_i2c_read(bus,
+ ASPEED_I2C_FUN_CTRL_REG) |
+ ASPEED_I2CD_M_HIGH_SPEED_EN |
+ ASPEED_I2CD_M_SDA_DRIVE_1T_EN |
+ ASPEED_I2CD_SDA_DRIVE_1T_EN,
+ ASPEED_I2C_FUN_CTRL_REG);
+
+ aspeed_i2c_write(bus, 0x3, ASPEED_I2C_AC_TIMING_REG2);
+ aspeed_i2c_write(bus, aspeed_i2c_get_clk_reg_val(divider_ratio),
+ ASPEED_I2C_AC_TIMING_REG1);
+ } else {
+ aspeed_i2c_write(bus, aspeed_i2c_get_clk_reg_val(divider_ratio),
+ ASPEED_I2C_AC_TIMING_REG1);
+ aspeed_i2c_write(bus, ASPEED_NO_TIMEOUT_CTRL,
+ ASPEED_I2C_AC_TIMING_REG2);
+ }
+
+ return 0;
+}
+
+static int aspeed_i2c_probe_bus(struct platform_device *pdev)
+{
+ struct aspeed_i2c_bus *bus;
+ struct resource *res;
+ int ret;
+
+ bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
+ if (!bus)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ bus->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(bus->base))
+ return PTR_ERR(bus->base);
+
+ bus->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
+ ret = devm_request_irq(&pdev->dev, bus->irq, aspeed_i2c_bus_irq,
+ IRQF_SHARED, dev_name(&pdev->dev), bus);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "failed to request interrupt\n");
+ return ret;
+ }
+
+ /* Initialize the I2C adapter */
+ spin_lock_init(&bus->lock);
+ init_completion(&bus->cmd_complete);
+ bus->adap.owner = THIS_MODULE;
+ bus->adap.retries = 0;
+ bus->adap.timeout = 5 * HZ;
+ bus->adap.algo = &aspeed_i2c_algo;
+ bus->adap.algo_data = bus;
+ bus->adap.dev.parent = &pdev->dev;
+ bus->adap.dev.of_node = pdev->dev.of_node;
+ snprintf(bus->adap.name, sizeof(bus->adap.name), "Aspeed i2c");
+
+ bus->dev = &pdev->dev;
+
+ /* reset device: disable master & slave functions */
+ aspeed_i2c_write(bus, 0, ASPEED_I2C_FUN_CTRL_REG);
+
+ ret = aspeed_i2c_init_clk(bus, pdev);
+ if (ret < 0)
+ return ret;
+
+ /* Enable Master Mode */
+ aspeed_i2c_write(bus, aspeed_i2c_read(bus, ASPEED_I2C_FUN_CTRL_REG) |
+ ASPEED_I2CD_MASTER_EN |
+ ASPEED_I2CD_MULTI_MASTER_DIS, ASPEED_I2C_FUN_CTRL_REG);
+
+ /* Set interrupt generation of I2C controller */
+ aspeed_i2c_write(bus, ASPEED_I2CD_INTR_ALL, ASPEED_I2C_INTR_CTRL_REG);
+
+ ret = i2c_add_adapter(&bus->adap);
+ if (ret < 0)
+ return ret;
+
+ platform_set_drvdata(pdev, bus);
+
+ dev_info(bus->dev, "i2c bus %d registered, irq %d\n",
+ bus->adap.nr, bus->irq);
+
+ return 0;
+}
+
+static int aspeed_i2c_remove_bus(struct platform_device *pdev)
+{
+ struct aspeed_i2c_bus *bus = platform_get_drvdata(pdev);
+
+ i2c_del_adapter(&bus->adap);
+
+ return 0;
+}
+
+static const struct of_device_id aspeed_i2c_bus_of_table[] = {
+ { .compatible = "aspeed,ast2400-i2c-bus", },
+ { .compatible = "aspeed,ast2500-i2c-bus", },
+ { },
+};
+MODULE_DEVICE_TABLE(of, aspeed_i2c_bus_of_table);
+
+static struct platform_driver aspeed_i2c_bus_driver = {
+ .probe = aspeed_i2c_probe_bus,
+ .remove = aspeed_i2c_remove_bus,
+ .driver = {
+ .name = "ast-i2c-bus",
+ .of_match_table = aspeed_i2c_bus_of_table,
+ },
+};
+
+/*
+ * The aspeed chip provides a single hardware interrupt for all of the I2C
+ * busses, so we use a dummy interrupt chip to translate this single interrupt
+ * into multiple interrupts, each associated with a single I2C bus.
+ */
+static void aspeed_i2c_controller_irq(struct irq_desc *desc)
+{
+ struct aspeed_i2c_controller *c = irq_desc_get_handler_data(desc);
+ struct irq_chip *chip = irq_desc_get_chip(desc);
+ unsigned long p, status;
+ unsigned int bus_irq;
+
+ chained_irq_enter(chip, desc);
+ status = readl(c->base);
+ for_each_set_bit(p, &status, ASPEED_I2C_NUM_BUS) {
+ bus_irq = irq_find_mapping(c->irq_domain, p);
+ generic_handle_irq(bus_irq);
+ }
+ chained_irq_exit(chip, desc);
+}
+
+/*
+ * Set simple handler and mark IRQ as valid. Nothing interesting to do here
+ * since we are using a dummy interrupt chip.
+ */
+static int aspeed_i2c_map_irq_domain(struct irq_domain *domain,
+ unsigned int irq, irq_hw_number_t hwirq)
+{
+ irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
+ irq_set_chip_data(irq, domain->host_data);
+
+ return 0;
+}
+
+static const struct irq_domain_ops aspeed_i2c_irq_domain_ops = {
+ .map = aspeed_i2c_map_irq_domain,
+};
+
+static int aspeed_i2c_probe_controller(struct platform_device *pdev)
+{
+ struct aspeed_i2c_controller *controller;
+ struct device_node *np;
+ struct resource *res;
+
+ controller = devm_kzalloc(&pdev->dev, sizeof(*controller), GFP_KERNEL);
+ if (!controller)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ controller->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(controller->base))
+ return PTR_ERR(controller->base);
+
+ controller->irq = platform_get_irq(pdev, 0);
+ if (controller->irq < 0)
+ return -ENXIO;
+
+ controller->irq_domain = irq_domain_add_linear(pdev->dev.of_node,
+ ASPEED_I2C_NUM_BUS, &aspeed_i2c_irq_domain_ops, NULL);
+ if (!controller->irq_domain)
+ return -ENOMEM;
+
+ controller->irq_domain->name = "ast-i2c-domain";
+
+ irq_set_chained_handler_and_data(controller->irq,
+ aspeed_i2c_controller_irq, controller);
+
+ controller->dev = &pdev->dev;
+
+ platform_set_drvdata(pdev, controller);
+
+ dev_info(controller->dev, "i2c controller registered, irq %d\n",
+ controller->irq);
+
+ for_each_child_of_node(pdev->dev.of_node, np) {
+ of_platform_device_create(np, NULL, &pdev->dev);
+ }
+
+ return 0;
+}
+
+static int aspeed_i2c_remove_controller(struct platform_device *pdev)
+{
+ struct aspeed_i2c_controller *controller = platform_get_drvdata(pdev);
+
+ irq_domain_remove(controller->irq_domain);
+
+ return 0;
+}
+
+static const struct of_device_id aspeed_i2c_controller_of_table[] = {
+ { .compatible = "aspeed,ast2400-i2c-controller", },
+ { .compatible = "aspeed,ast2500-i2c-controller", },
+ { },
+};
+MODULE_DEVICE_TABLE(of, aspeed_i2c_controller_of_table);
+
+static struct platform_driver aspeed_i2c_controller_driver = {
+ .probe = aspeed_i2c_probe_controller,
+ .remove = aspeed_i2c_remove_controller,
+ .driver = {
+ .name = "ast-i2c-controller",
+ .of_match_table = aspeed_i2c_controller_of_table,
+ },
+};
+
+static int __init aspeed_i2c_driver_init(void)
+{
+ int ret;
+
+ ret = platform_driver_register(&aspeed_i2c_controller_driver);
+ if (ret < 0) {
+ platform_driver_unregister(&aspeed_i2c_controller_driver);
+ return ret;
+ }
+
+ ret = platform_driver_register(&aspeed_i2c_bus_driver);
+
+ if (ret < 0) {
+ platform_driver_unregister(&aspeed_i2c_bus_driver);
+ platform_driver_unregister(&aspeed_i2c_controller_driver);
+ return ret;
+ }
+
+ return 0;
+}
+module_init(aspeed_i2c_driver_init);
+
+static void __exit aspeed_i2c_driver_exit(void)
+{
+ platform_driver_unregister(&aspeed_i2c_bus_driver);
+ platform_driver_unregister(&aspeed_i2c_controller_driver);
+}
+module_exit(aspeed_i2c_driver_exit);
+
+MODULE_AUTHOR("Brendan Higgins <brendanhiggins@google.com>");
+MODULE_DESCRIPTION("Aspeed I2C Bus Driver");
+MODULE_LICENSE("GPL");
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v5 2/2] i2c: aspeed: added documentation for Aspeed I2C driver
From: Brendan Higgins @ 2016-11-30 1:00 UTC (permalink / raw)
To: wsa-z923LK4zBo2bacvFa/9K2g, vz-ChpfBGZJDbMAvxtiuMwx3w,
clg-Bxea+6Xhats, mouse-Pma6HLj0uuo,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, joel-U3u1mxZcP9KHXe+LvDLADg,
openbmc-uLR06cmDAlY/bJ5BZ2RsiQ, Brendan Higgins
In-Reply-To: <1480467618-7497-1-git-send-email-brendanhiggins-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Added device tree binding documentation for Aspeed I2C controller and
busses.
Signed-off-by: Brendan Higgins <brendanhiggins-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
Changes for v2:
- None
Changes for v3:
- Removed reference to "bus" device tree param
Changes for v4:
- None
Changes for v5:
- None
---
.../devicetree/bindings/i2c/i2c-aspeed.txt | 61 ++++++++++++++++++++++
1 file changed, 61 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
new file mode 100644
index 0000000..dd11a97
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
@@ -0,0 +1,61 @@
+Device tree configuration for the I2C controller and busses on the AST24XX
+and AST25XX SoCs.
+
+Controller:
+
+ Required Properties:
+ - #address-cells : should be 1
+ - #size-cells : should be 1
+ - #interrupt-cells : should be 1
+ - compatible : should be "aspeed,ast2400-i2c-controller"
+ or "aspeed,ast2500-i2c-controller"
+ - reg : address start and range of controller
+ - ranges : defines address offset and range for busses
+ - interrupts : interrupt number
+ - clocks : root clock of bus, should reference the APB
+ clock
+ - clock-ranges : specifies that child busses can inherit clocks
+ - interrupt-controller : denotes that the controller receives and fires
+ new interrupts for child busses
+
+Bus:
+
+ Required Properties:
+ - #address-cells : should be 1
+ - #size-cells : should be 0
+ - reg : address offset and range of bus
+ - compatible : should be "aspeed,ast2400-i2c-bus"
+ or "aspeed,ast2500-i2c-bus"
+ - interrupts : interrupt number
+
+ Optional Properties:
+ - clock-frequency : frequency of the bus clock in Hz
+ defaults to 100 kHz when not specified
+
+Example:
+
+i2c: i2c@1e78a000 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ #interrupt-cells = <1>;
+
+ compatible = "aspeed,ast2400-i2c-controller";
+ reg = <0x1e78a000 0x40>;
+ ranges = <0 0x1e78a000 0x1000>;
+ interrupts = <12>;
+ clocks = <&clk_apb>;
+ clock-ranges;
+ interrupt-controller;
+
+ i2c0: i2c-bus@40 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x40 0x40>;
+ compatible = "aspeed,ast2400-i2c-bus";
+ clock-frequency = <100000>;
+ status = "disabled";
+ interrupts = <0>;
+ interrupt-parent = <&i2c>;
+ };
+};
+
--
2.8.0.rc3.226.g39d4020
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH 1/2] PM / Domains: Introduce domain-performance-state binding
From: Stephen Boyd @ 2016-11-30 1:08 UTC (permalink / raw)
To: Viresh Kumar
Cc: Kevin Hilman, Vincent Guittot, Rob Herring, Rafael Wysocki,
linaro-kernel@lists.linaro.org, linux-pm@vger.kernel.org,
linux-kernel, Mark Rutland, Ulf Hansson, Lina Iyer,
devicetree@vger.kernel.org, Nayak Rajendra
In-Reply-To: <20161129065726.GG3288@vireshk-i7>
On 11/29, Viresh Kumar wrote:
> On 28-11-16, 10:27, Stephen Boyd wrote:
> > On 11/23/2016 08:40 PM, Viresh Kumar wrote:
> > > But even in these cases we wouldn't be using the voltage values within the
> > > kernel as we will be giving only a performance state to the M3 core, right?
> >
> > Nope. In these cases we need to set a certain voltage and we do that by
> > requesting it via the M3 core.
>
> Don't we need something like this then ?
Perhaps. One question is if we consider a shared regulator as a
regulator in the kernel, or if we want to hide the regulator
behind some other API that aggregates the users of the voltage. I
don't see how to draw the line clearly between a regulator and a
power domain that modifies a regulator underneath. It seems like
everything that's using a regulator on the SoC could be using a
power domain instead and then we could be aggregating the voltage
requirements outside of the regulator APIs.
The only other way I can think of doing it is by having the
voltages in the OPP tables for each device. That gets sort of
messy though because all the devices calling
regulator_set_voltage() have to set the min voltage to be their
required voltage and the max to be the global max voltage on the
system. Otherwise a higher voltage may not be used while it may
be required. Of course, we could encode that as the last value in
the triplet and everything works.
>
> parent: power-controller@12340000 {
> compatible = "foo,power-controller";
> reg = <0x12340000 0x1000>;
> #power-domain-cells = <0>;
> domain-performance-states = <&perf_state0>;
> };
>
> perf_state0: performance_states {
> pstate1: pstate@1 {
> index = <1>;
> /* Optional */
> microvolt = <970000 975000 985000>;
> };
> pstate2: pstate@2 {
> index = <2>;
> /* Optional */
> microvolt = <970000 975000 985000>;
> };
> pstate3: pstate@3 {
> index = <3>;
> /* Optional */
> microvolt = <970000 975000 985000>;
> };
> }
>
> cpus {
> cpu@0 {
> ...
> power-domain = <&parent>;
> operating-points-v2 = <&cpu0_opp_table>;
> };
> };
>
> cpu0_opp_table: opp_table0 {
> compatible = "operating-points-v2";
> opp-shared;
>
> opp@1000000000 {
> opp-hz = /bits/ 64 <1000000000>;
> domain-performance-state = <&pstate1>;
What do we do if the device is part of multiple domains? For
example it may be part of two power domains for different pieces
of the silicon within one device, and we may want to
independently control those domains depending on the clock
frequency.
> };
> opp@1100000000 {
> opp-hz = /bits/ 64 <1100000000>;
> domain-performance-state = <&pstate2>;
> };
> opp@1200000000 {
> opp-hz = /bits/ 64 <1200000000>;
> domain-performance-state = <&pstate3>;
> };
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [alsa-devel] [PATCH v2] clkdev: add devm_of_clk_get()
From: Kuninori Morimoto @ 2016-11-30 1:08 UTC (permalink / raw)
To: Stephen Boyd
Cc: Rob Herring, Linux-ALSA, Linux-DT, Michael Turquette,
Russell King, Linux-Kernel, Mark Brown,
linux-clk-u79uwXL29TY76Z2rM5mHXA, Linux-ARM
In-Reply-To: <20161129210556.GC6095-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Hi Stephen
Thank you for your feedback.
> > > > sound_soc {
> > > > clocks = <&xxx>, <&xxx>;
> > > > clock-names = "cpu", "codec";
> > > > ...
> > > > cpu {
> > > > ...
> > > > };
> > > > codec {
> > > > ...
> > > > };
> > > > };
(snip)
> The problem is that it encourages the use of of_clk_get() when
> clk_get() is more desirable. Ideally of_clk_get() is never used
> when a device exists. In this case, it seems like we need to
> support it though, hence the suggestion of having a
> devm_get_clk_from_child() API, that explicitly reads as "get a
> clock from a child node of this device". The distinction is
> important, because of_clk_get() should rarely be used.
I understand your point, but I think devm_get_clk_from_child()
needs new DT setings, and it can't keep compatibility, or
it makes driver complex.
I think it is nice to have. but, I want to keep current style.
Thus, I will try to use current of_clk_get() as-is, and
call clk_free() somehow in this driver.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v3 0/3] Altera Cyclone Passive Serial SPI FPGA Manager
From: Joshua Clayton @ 2016-11-30 1:11 UTC (permalink / raw)
To: Alan Tull, Moritz Fischer
Cc: Rob Herring, Mark Rutland, Russell King, Joshua Clayton,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
This series adds an FPGA manager for Altera cyclone FPGAs
that can program them using an spi port and a couple of gpios, using
Alteras passive serial protocol.
Changes from v2:
- Merged patch 3 and 4 as suggested in review by Moritz Fischer
- Changed FPGA_MIN_DELAY from 250 to 50 ms is the time advertized by
Altera. This now works, as we don't assume it is done
Changes from v1:
- Changed the name from cyclone-spi-fpga-mgr to cyclone-ps-spi-fpga-mgr
This name change was requested by Alan Tull, to be specific about which
programming method is being employed on the fpga.
- Changed the name of the reset-gpio to config-gpio to closer match the
way the pins are described in the Altera manual
- Moved MODULE_LICENCE, _AUTHOR, and _DESCRIPTION to the bottom
- Added a bitrev8x4() function to the bitrev headers and implemented ARM
const, runtime, and ARM specific faster versions (This may end up
needing to be a standalone patch)
- Moved the bitswapping into cyclonespi_write(), as requested.
This falls short of my desired generic lsb first spi support, but is a step
in that direction.
- Fixed whitespace problems introduced during refactoring
- Replaced magic number for initial delay with a descriptive macro
- Poll the fpga to see when it is ready rather than a fixed 1 ms sleep
Joshua Clayton (3):
lib: add bitrev8x4()
doc: dt: add cyclone-spi binding document
fpga manager: Add cyclone-ps-spi driver for Altera FPGAs
.../bindings/fpga/cyclone-ps-spi-fpga-mgr.txt | 23 +++
arch/arm/include/asm/bitrev.h | 5 +
drivers/fpga/Kconfig | 7 +
drivers/fpga/Makefile | 1 +
drivers/fpga/cyclone-ps-spi.c | 176 +++++++++++++++++++++
include/linux/bitrev.h | 26 +++
6 files changed, 238 insertions(+)
create mode 100644 Documentation/devicetree/bindings/fpga/cyclone-ps-spi-fpga-mgr.txt
create mode 100644 drivers/fpga/cyclone-ps-spi.c
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v3 1/3] lib: add bitrev8x4()
From: Joshua Clayton @ 2016-11-30 1:11 UTC (permalink / raw)
To: Alan Tull, Moritz Fischer
Cc: Rob Herring, Mark Rutland, Russell King, Joshua Clayton,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <cover.1480467185.git.stillcompiling-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Add a function to reverse bytes within a 32 bit word.
This function is more efficient than using the 8 bit version when
iterating over an array
Signed-off-by: Joshua Clayton <stillcompiling-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
arch/arm/include/asm/bitrev.h | 5 +++++
include/linux/bitrev.h | 26 ++++++++++++++++++++++++++
2 files changed, 31 insertions(+)
diff --git a/arch/arm/include/asm/bitrev.h b/arch/arm/include/asm/bitrev.h
index ec291c3..6d2e9ca 100644
--- a/arch/arm/include/asm/bitrev.h
+++ b/arch/arm/include/asm/bitrev.h
@@ -17,4 +17,9 @@ static __always_inline __attribute_const__ u8 __arch_bitrev8(u8 x)
return __arch_bitrev32((u32)x) >> 24;
}
+static __always_inline __attribute_const__ u32 __arch_bitrev8x4(u32 x)
+{
+ __asm__ ("rbit %0, %1; rev %0, %0" : "=r" (x) : "r" (x));
+}
+
#endif
diff --git a/include/linux/bitrev.h b/include/linux/bitrev.h
index fb790b8..b1cfa1a 100644
--- a/include/linux/bitrev.h
+++ b/include/linux/bitrev.h
@@ -9,6 +9,7 @@
#define __bitrev32 __arch_bitrev32
#define __bitrev16 __arch_bitrev16
#define __bitrev8 __arch_bitrev8
+#define __bitrev8x4 __arch_bitrev8x4
#else
extern u8 const byte_rev_table[256];
@@ -27,6 +28,14 @@ static inline u32 __bitrev32(u32 x)
return (__bitrev16(x & 0xffff) << 16) | __bitrev16(x >> 16);
}
+static inline u32 __bitrev8x4(u32 x)
+{
+ return(__bitrev8(x & 0xff) |
+ (__bitrev8((x >> 8) & 0xff) << 8) |
+ (__bitrev8((x >> 16) & 0xff) << 16) |
+ (__bitrev8((x >> 24) & 0xff) << 24));
+}
+
#endif /* CONFIG_HAVE_ARCH_BITREVERSE */
#define __constant_bitrev32(x) \
@@ -50,6 +59,15 @@ static inline u32 __bitrev32(u32 x)
__x; \
})
+#define __constant_bitrev8x4(x) \
+({ \
+ u32 __x = x; \
+ __x = ((__x & (u32)0xF0F0F0F0UL) >> 4) | ((__x & (u32)0x0F0F0F0FUL) << 4); \
+ __x = ((__x & (u32)0xCCCCCCCCUL) >> 2) | ((__x & (u32)0x33333333UL) << 2); \
+ __x = ((__x & (u32)0xAAAAAAAAUL) >> 1) | ((__x & (u32)0x55555555UL) << 1); \
+ __x; \
+})
+
#define __constant_bitrev8(x) \
({ \
u8 __x = x; \
@@ -75,6 +93,14 @@ static inline u32 __bitrev32(u32 x)
__bitrev16(__x); \
})
+#define bitrev8x4(x) \
+({ \
+ u32 __x = x; \
+ __builtin_constant_p(__x) ? \
+ __constant_bitrev8x4(__x) : \
+ __bitrev8x4(__x); \
+})
+
#define bitrev8(x) \
({ \
u8 __x = x; \
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v3 2/3] doc: dt: add cyclone-spi binding document
From: Joshua Clayton @ 2016-11-30 1:11 UTC (permalink / raw)
To: Alan Tull, Moritz Fischer
Cc: Rob Herring, Mark Rutland, Russell King, Joshua Clayton,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <cover.1480467185.git.stillcompiling-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Describe a cyclonei-ps-spi devicetree entry, required features
Signed-off-by: Joshua Clayton <stillcompiling-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
.../bindings/fpga/cyclone-ps-spi-fpga-mgr.txt | 23 ++++++++++++++++++++++
1 file changed, 23 insertions(+)
create mode 100644 Documentation/devicetree/bindings/fpga/cyclone-ps-spi-fpga-mgr.txt
diff --git a/Documentation/devicetree/bindings/fpga/cyclone-ps-spi-fpga-mgr.txt b/Documentation/devicetree/bindings/fpga/cyclone-ps-spi-fpga-mgr.txt
new file mode 100644
index 0000000..c942281
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/cyclone-ps-spi-fpga-mgr.txt
@@ -0,0 +1,23 @@
+Altera Cyclone Passive Serial SPI FPGA Manager
+
+Altera Cyclone FPGAs support a method of loading the bitstream over what is
+referred to as "passive serial".
+The passive serial link is not technically spi, and might require extra
+circuits in order to play nicely with other spi slaves on the same bus.
+
+See https://www.altera.com/literature/hb/cyc/cyc_c51013.pdf
+
+Required properties:
+- compatible : should contain "altr,cyclone-ps-spi-fpga-mgr"
+- reg : spi slave id of the fpga
+- config-gpio : config pin (referred to as nCONFIG in the cyclone manual)
+- status-gpio : status pin (referred to as nSTATUS in the cyclone manual)
+
+Example:
+ fpga_spi: evi-fpga-spi@0 {
+ compatible = "altr,cyclone-ps-spi-fpga-mgr";
+ spi-max-frequency = <20000000>;
+ reg = <0>;
+ config-gpio = <&gpio4 9 GPIO_ACTIVE_HIGH>;
+ status-gpio = <&gpio4 11 GPIO_ACTIVE_HIGH>;
+ };
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v3 3/3] fpga manager: Add cyclone-ps-spi driver for Altera FPGAs
From: Joshua Clayton @ 2016-11-30 1:11 UTC (permalink / raw)
To: Alan Tull, Moritz Fischer
Cc: Rob Herring, Mark Rutland, Russell King, Joshua Clayton,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <cover.1480467185.git.stillcompiling-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
cyclone-ps-spi loads FPGA firmware over spi, using the "passive serial"
interface on Altera Cyclone FPGAS.
This is one of the simpler ways to set up an FPGA at runtime.
The signal interface is close to unidirectional spi with lsb first.
Signed-off-by: Joshua Clayton <stillcompiling-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
drivers/fpga/Kconfig | 7 ++
drivers/fpga/Makefile | 1 +
drivers/fpga/cyclone-ps-spi.c | 176 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 184 insertions(+)
create mode 100644 drivers/fpga/cyclone-ps-spi.c
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index cd84934..2462707 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -13,6 +13,13 @@ config FPGA
if FPGA
+config FPGA_MGR_CYCLONE_PS_SPI
+ tristate "Altera Cyclone FPGA Passive Serial over SPI"
+ depends on SPI
+ help
+ FPGA manager driver support for Altera Cyclone using the
+ passive serial interface over SPI
+
config FPGA_MGR_SOCFPGA
tristate "Altera SOCFPGA FPGA Manager"
depends on ARCH_SOCFPGA
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 8d83fc6..8f93930 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -6,5 +6,6 @@
obj-$(CONFIG_FPGA) += fpga-mgr.o
# FPGA Manager Drivers
+obj-$(CONFIG_FPGA_MGR_CYCLONE_PS_SPI) += cyclone-ps-spi.o
obj-$(CONFIG_FPGA_MGR_SOCFPGA) += socfpga.o
obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o
diff --git a/drivers/fpga/cyclone-ps-spi.c b/drivers/fpga/cyclone-ps-spi.c
new file mode 100644
index 0000000..57a520d
--- /dev/null
+++ b/drivers/fpga/cyclone-ps-spi.c
@@ -0,0 +1,176 @@
+/**
+ * Copyright (c) 2015 United Western Technologies, Corporation
+ *
+ * Joshua Clayton <stillcompiling-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ *
+ * Manage Altera fpga firmware that is loaded over spi.
+ * Firmware must be in binary "rbf" format.
+ * Works on Cyclone V. Should work on cyclone series.
+ * May work on other Altera fpgas.
+ *
+ */
+
+#include <linux/bitrev.h>
+#include <linux/delay.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of_gpio.h>
+#include <linux/spi/spi.h>
+#include <linux/sizes.h>
+
+#define FPGA_RESET_TIME 50 /* time in usecs to trigger FPGA config */
+#define FPGA_MIN_DELAY 50 /* min usecs to wait for config status */
+#define FPGA_MAX_DELAY 1000 /* max usecs to wait for config status */
+
+struct cyclonespi_conf {
+ struct gpio_desc *config;
+ struct gpio_desc *status;
+ struct spi_device *spi;
+};
+
+static const struct of_device_id of_ef_match[] = {
+ { .compatible = "altr,cyclone-ps-spi-fpga-mgr", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, of_ef_match);
+
+static enum fpga_mgr_states cyclonespi_state(struct fpga_manager *mgr)
+{
+ return mgr->state;
+}
+
+static int cyclonespi_write_init(struct fpga_manager *mgr, u32 flags,
+ const char *buf, size_t count)
+{
+ struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
+ int i;
+
+ if (flags & FPGA_MGR_PARTIAL_RECONFIG) {
+ dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
+ return -EINVAL;
+ }
+
+ gpiod_set_value(conf->config, 0);
+ usleep_range(FPGA_RESET_TIME, FPGA_RESET_TIME + 20);
+ if (gpiod_get_value(conf->status) == 1) {
+ dev_err(&mgr->dev, "Status pin should be low.\n");
+ return -EIO;
+ }
+
+ gpiod_set_value(conf->config, 1);
+ for (i = 0; i < (FPGA_MAX_DELAY / FPGA_MIN_DELAY); i++) {
+ usleep_range(FPGA_MIN_DELAY, FPGA_MIN_DELAY + 20);
+ if (gpiod_get_value(conf->status))
+ return 0;
+ }
+
+ dev_err(&mgr->dev, "Status pin not ready.\n");
+ return -EIO;
+}
+
+static void rev_buf(void *buf, size_t len)
+{
+ u32 *fw32 = (u32 *)buf;
+ const u32 *fw_end = (u32 *)(buf + len);
+
+ /* set buffer to lsb first */
+ while (fw32 < fw_end) {
+ *fw32 = bitrev8x4(*fw32);
+ fw32++;
+ }
+}
+
+static int cyclonespi_write(struct fpga_manager *mgr, const char *buf,
+ size_t count)
+{
+ struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
+ const char *fw_data = buf;
+ const char *fw_data_end = fw_data + count;
+
+ while (fw_data < fw_data_end) {
+ int ret;
+ size_t stride = min(fw_data_end - fw_data, SZ_4K);
+
+ rev_buf((void *)fw_data, stride);
+ ret = spi_write(conf->spi, fw_data, stride);
+ if (ret) {
+ dev_err(&mgr->dev, "spi error in firmware write: %d\n",
+ ret);
+ return ret;
+ }
+ fw_data += stride;
+ }
+
+ return 0;
+}
+
+static int cyclonespi_write_complete(struct fpga_manager *mgr, u32 flags)
+{
+ struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
+
+ if (gpiod_get_value(conf->status) == 0) {
+ dev_err(&mgr->dev, "Error during configuration.\n");
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static const struct fpga_manager_ops cyclonespi_ops = {
+ .state = cyclonespi_state,
+ .write_init = cyclonespi_write_init,
+ .write = cyclonespi_write,
+ .write_complete = cyclonespi_write_complete,
+};
+
+static int cyclonespi_probe(struct spi_device *spi)
+{
+ struct cyclonespi_conf *conf = devm_kzalloc(&spi->dev, sizeof(*conf),
+ GFP_KERNEL);
+
+ if (!conf)
+ return -ENOMEM;
+
+ conf->spi = spi;
+ conf->config = devm_gpiod_get(&spi->dev, "config", GPIOD_OUT_LOW);
+ if (IS_ERR(conf->config)) {
+ dev_err(&spi->dev, "Failed to get config gpio: %ld\n",
+ PTR_ERR(conf->config));
+ return PTR_ERR(conf->config);
+ }
+
+ conf->status = devm_gpiod_get(&spi->dev, "status", GPIOD_IN);
+ if (IS_ERR(conf->status)) {
+ dev_err(&spi->dev, "Failed to get status gpio: %ld\n",
+ PTR_ERR(conf->status));
+ return PTR_ERR(conf->status);
+ }
+
+ return fpga_mgr_register(&spi->dev,
+ "Altera Cyclone PS SPI FPGA Manager",
+ &cyclonespi_ops, conf);
+}
+
+static int cyclonespi_remove(struct spi_device *spi)
+{
+ fpga_mgr_unregister(&spi->dev);
+
+ return 0;
+}
+
+static struct spi_driver cyclonespi_driver = {
+ .driver = {
+ .name = "cyclone-ps-spi",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(of_ef_match),
+ },
+ .probe = cyclonespi_probe,
+ .remove = cyclonespi_remove,
+};
+
+module_spi_driver(cyclonespi_driver)
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Joshua Clayton <stillcompiling-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
+MODULE_DESCRIPTION("Module to load Altera FPGA firmware over spi");
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH net-next v3 0/4] Fix OdroidC2 Gigabit Tx link issue
From: David Miller @ 2016-11-30 1:13 UTC (permalink / raw)
To: f.fainelli-Re5JQEeQqe8AvxtiuMwx3w
Cc: jbrunet-rdvid1DuHRBWk0Htik3J/w, netdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, carlo-KA+7E9HrN00dnm+yROfE0A,
khilman-rdvid1DuHRBWk0Htik3J/w, peppe.cavallaro-qxv4g6HH51o,
alexandre.torgue-qxv4g6HH51o,
martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg,
neolynx-Re5JQEeQqe8AvxtiuMwx3w, andrew-g2DYL2Zd6BY,
narmstrong-rdvid1DuHRBWk0Htik3J/w,
linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <e14a3b0c-dc34-be14-48b3-518a0ad0c080-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
From: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Tue, 29 Nov 2016 16:43:20 -0800
> On 11/29/2016 04:38 PM, David Miller wrote:
>> From: Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>> Date: Mon, 28 Nov 2016 10:46:45 +0100
>>
>>> This patchset fixes an issue with the OdroidC2 board (DWMAC + RTL8211F).
>>> The platform seems to enter LPI on the Rx path too often while performing
>>> relatively high TX transfer. This eventually break the link (both Tx and
>>> Rx), and require to bring the interface down and up again to get the Rx
>>> path working again.
>>>
>>> The root cause of this issue is not fully understood yet but disabling EEE
>>> advertisement on the PHY prevent this feature to be negotiated.
>>> With this change, the link is stable and reliable, with the expected
>>> throughput performance.
>>>
>>> The patchset adds options in the generic phy driver to disable EEE
>>> advertisement, through device tree. The way it is done is very similar
>>> to the handling of the max-speed property.
>>
>> Patches 1-3 applied to net-next, thanks.
>
> Meh, there was a v4 submitted shortly after, and I objected to the whole
> idea of using that kind of Device Tree properties to disable EEE, we can
> send reverts though..
Sorry, I lost this in all the discussion, I can revert.
Just send me a revert of the entire merge commit
a152c91889556df17ca6d8ea134fb2cb4ac9f893 with a short
description of why and I'll apply it.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net-next v3 0/4] Fix OdroidC2 Gigabit Tx link issue
From: Florian Fainelli @ 2016-11-30 1:15 UTC (permalink / raw)
To: David Miller
Cc: jbrunet-rdvid1DuHRBWk0Htik3J/w, netdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, carlo-KA+7E9HrN00dnm+yROfE0A,
khilman-rdvid1DuHRBWk0Htik3J/w, peppe.cavallaro-qxv4g6HH51o,
alexandre.torgue-qxv4g6HH51o,
martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg,
neolynx-Re5JQEeQqe8AvxtiuMwx3w, andrew-g2DYL2Zd6BY,
narmstrong-rdvid1DuHRBWk0Htik3J/w,
linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161129.201331.2207317476589573523.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
On 11/29/2016 05:13 PM, David Miller wrote:
> From: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Date: Tue, 29 Nov 2016 16:43:20 -0800
>
>> On 11/29/2016 04:38 PM, David Miller wrote:
>>> From: Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>> Date: Mon, 28 Nov 2016 10:46:45 +0100
>>>
>>>> This patchset fixes an issue with the OdroidC2 board (DWMAC + RTL8211F).
>>>> The platform seems to enter LPI on the Rx path too often while performing
>>>> relatively high TX transfer. This eventually break the link (both Tx and
>>>> Rx), and require to bring the interface down and up again to get the Rx
>>>> path working again.
>>>>
>>>> The root cause of this issue is not fully understood yet but disabling EEE
>>>> advertisement on the PHY prevent this feature to be negotiated.
>>>> With this change, the link is stable and reliable, with the expected
>>>> throughput performance.
>>>>
>>>> The patchset adds options in the generic phy driver to disable EEE
>>>> advertisement, through device tree. The way it is done is very similar
>>>> to the handling of the max-speed property.
>>>
>>> Patches 1-3 applied to net-next, thanks.
>>
>> Meh, there was a v4 submitted shortly after, and I objected to the whole
>> idea of using that kind of Device Tree properties to disable EEE, we can
>> send reverts though..
>
> Sorry, I lost this in all the discussion, I can revert.
Yeah, I can understand why, these freaking PHYs tend to generate a lot
of noise and discussion...
>
> Just send me a revert of the entire merge commit
> a152c91889556df17ca6d8ea134fb2cb4ac9f893 with a short
> description of why and I'll apply it.
OK, I will talk with Jerome first to make sure that we are in agreement
with the solution to deploy to fix the OdroidC2 problem first.
Thanks!
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 2/2] mtd: spi-nor: add rockchip serial flash controller driver
From: Shawn Lin @ 2016-11-30 1:17 UTC (permalink / raw)
To: Marek Vasut, David Woodhouse, Brian Norris
Cc: shawn.lin-TNX95d0MmH7DzftRWevZcw, Cyrille Pitchen, Rob Herring,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Heiko Stuebner
In-Reply-To: <62baf961-e84f-24a4-4345-05a51f351c01-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
在 2016/11/25 21:52, Marek Vasut 写道:
> On 11/18/2016 03:59 AM, Shawn Lin wrote:
>> Add rockchip serial flash controller driver
>>
>> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>
> [...]
>
>> +enum rockchip_sfc_iftype {
>> + IF_TYPE_STD,
>> + IF_TYPE_DUAL,
>> + IF_TYPE_QUAD,
>> +};
>> +
>> +struct rockchip_sfc;
>> +struct rockchip_sfc_chip_priv {
>> + u8 cs;
>> + u32 clk_rate;
>> + struct spi_nor nor;
>> + struct rockchip_sfc *sfc;
>> +};
>> +
>> +struct rockchip_sfc {
>> + struct device *dev;
>> + struct mutex lock;
>> + void __iomem *regbase;
>> + struct clk *hclk;
>> + struct clk *clk;
>> + /* virtual mapped addr for dma_buffer */
>> + void *buffer;
>> + dma_addr_t dma_buffer;
>> + struct completion cp;
>> + struct rockchip_sfc_chip_priv flash[SFC_MAX_CHIPSELECT_NUM];
>> + u32 num_chip;
>> + bool use_dma;
>> + /* use negative edge of hclk to latch data */
>> + bool negative_edge;
>> +};
>> +
>> +static int get_if_type(enum read_mode flash_read)
>> +{
>> + enum rockchip_sfc_iftype if_type;
>> +
>> + switch (flash_read) {
>> + case SPI_NOR_DUAL:
>> + if_type = IF_TYPE_DUAL;
>> + break;
>> + case SPI_NOR_QUAD:
>> + if_type = IF_TYPE_QUAD;
>> + break;
>> + case SPI_NOR_NORMAL:
>> + case SPI_NOR_FAST:
>> + if_type = IF_TYPE_STD;
>> + break;
>> + default:
>> + pr_err("unsupported SPI read mode\n");
>
> I'd switch this to dev_err() , so it's obvious from which device this
> error came. It's OK to pass in the sfc pointer.
Sure.
>
>> + return -EINVAL;
>> + }
>> +
>> + return if_type;
>> +}
>
> [...]
>
>> +static int rockchip_sfc_write_reg(struct spi_nor *nor, u8 opcode,
>> + u8 *buf, int len)
>> +{
>> + struct rockchip_sfc_chip_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> + u32 dwords, i;
>> +
>> + /* Align bytes to dwords */
>> + dwords = (len + 3) >> 2;
>> +
>> + for (i = 0; i < dwords; i++)
>> + writel_relaxed(*(buf + 4 * i), sfc->regbase + SFC_DATA);
>
> Can $buf be unaligned to 4-bytes ? :-)
Ah, yes, I will check this.
>
>> + return rockchip_sfc_op_reg(nor, opcode, len, SFC_CMD_DIR_WR);
>> +}
>> +
>> +static inline void rockchip_sfc_setup_transfer(struct spi_nor *nor,
>> + loff_t from_to,
>> + size_t len, u8 op_type)
>> +{
>> + struct rockchip_sfc_chip_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> + u32 reg;
>> + u8 if_type = 0;
>> +
>> + if (op_type == SFC_CMD_DIR_WR)
>> + reg = (nor->program_opcode & SFC_CMD_IDX_MASK) <<
>> + SFC_CMD_IDX_SHIFT;
>> + else
>> + reg = (nor->read_opcode & SFC_CMD_IDX_MASK) <<
>> + SFC_CMD_IDX_SHIFT;
>
> You can define some SFC_CMD_IDX(foo) to avoid this two-line reg assignment:
>
> #define SFC_CMD_IDX(opc) \
> ((opc) & SFC_CMD_IDX_MASK) << SFC_CMD_IDX_SHIFT)
>
> reg = SFC_CMD_IDX(nor->read_opcode);
Sure.
>
>> + reg |= op_type << SFC_CMD_DIR_SHIFT;
>> + reg |= (nor->addr_width == 4) ?
>> + SFC_CMD_ADDR_32BITS : SFC_CMD_ADDR_24BITS;
>> +
>> + if_type = get_if_type(nor->flash_read);
>> + writel_relaxed((if_type << SFC_CTRL_DATA_BITS_SHIFT) |
>> + (if_type << SFC_CTRL_ADDR_BITS_SHIFT) |
>> + (if_type << SFC_CTRL_CMD_BITS_SHIFT) |
>> + (sfc->negative_edge ? SFC_CTRL_PHASE_SEL_NEGETIVE : 0),
>> + sfc->regbase + SFC_CTRL);
>> +
>> + reg |= (priv->cs & SFC_CMD_CS_MASK) << SFC_CMD_CS_SHIFT;
>> + reg |= (len & SFC_CMD_TRAN_BYTES_MASK) << SFC_CMD_TRAN_BYTES_SHIFT;
>> +
>> + if (op_type == SFC_CMD_DIR_RD)
>> + reg |= SFC_CMD_DUMMY(nor->read_dummy);
>> +
>> + /* Should minus one as 0x0 means 1 bit flash address */
>> + writel_relaxed(nor->addr_width * 8 - 1, sfc->regbase + SFC_ABIT);
>> + writel_relaxed(reg, sfc->regbase + SFC_CMD);
>> + writel_relaxed(from_to, sfc->regbase + SFC_ADDR);
>> +}
>> +
>> +static int rockchip_sfc_dma_transfer(struct spi_nor *nor, loff_t from_to,
>> + dma_addr_t dma_buf, size_t len, u8 op_type)
>> +{
>> + struct rockchip_sfc_chip_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> + u32 reg;
>> + int err = 0;
>> +
>> + init_completion(&sfc->cp);
>> +
>> + writel_relaxed(SFC_ICLR_RX_FULL | SFC_ICLR_RX_UFLOW |
>> + SFC_ICLR_TX_OFLOW | SFC_ICLR_TX_EMPTY |
>> + SFC_ICLR_TRAN_FINISH | SFC_ICLR_BUS_ERR |
>> + SFC_ICLR_NSPI_ERR | SFC_ICLR_DMA,
>> + sfc->regbase + SFC_ICLR);
>> +
>> + /* Enable transfer complete interrupt */
>> + reg = readl_relaxed(sfc->regbase + SFC_IMR);
>> + reg &= ~SFC_IMR_TRAN_FINISH;
>> + writel_relaxed(reg, sfc->regbase + SFC_IMR);
>> +
>> + rockchip_sfc_setup_transfer(nor, from_to, len, op_type);
>> + writel_relaxed(dma_buf, sfc->regbase + SFC_DMA_ADDR);
>> +
>> + /*
>> + * Start dma but note that the sfc->dma_buffer is derived from
>> + * dmam_alloc_coherent so we don't actually need any sync operations
>> + * for coherent dma memory.
>> + */
>> + writel_relaxed(0x1, sfc->regbase + SFC_DMA_TRIGGER);
>> +
>> + /* Wait for the interrupt. */
>> + if (!wait_for_completion_timeout(&sfc->cp, msecs_to_jiffies(2000))) {
>> + dev_err(sfc->dev, "DMA wait for transfer finish timeout\n");
>> + err = -ETIMEDOUT;
>
> Don't you want to stop the DMA too ?
SFC_DMA_TRIGGER will be self-cleared after staring dma.
If any error occured, there is no a "STOP button" for us
to stop it but resetting the controller. I was considering
that the next transfer will check the controller's state and
reset the controller but I guess it would be nice to reset
it here in advance.
Will add a reset for error cases.
>
>> + }
>> +
>> + /* Disable transfer finish interrupt */
>> + reg = readl_relaxed(sfc->regbase + SFC_IMR);
>> + reg |= SFC_IMR_TRAN_FINISH;
>> + writel_relaxed(reg, sfc->regbase + SFC_IMR);
>> +
>> + if (err)
>> + return err;
>> +
>> + return rockchip_sfc_wait_op_finish(sfc);
>> +}
>> +
>> +static inline int rockchip_sfc_pio_write(struct rockchip_sfc *sfc, u_char *buf,
>> + size_t len)
>> +{
>> + u32 dwords, tx_wl, count, i;
>> + unsigned long timeout;
>> + int ret = 0;
>> + u32 *tbuf = (u32 *)buf;
>> +
>> + /*
>> + * Align bytes to dwords, although we will write some extra
>> + * bytes to fifo but the transfer bytes number in SFC_CMD
>> + * register will make sure we just send out the expected
>> + * byte numbers and the extra bytes will be clean before
>> + * setting up the next transfer. We should always round up
>> + * to align to DWORD as the ahb for Rockchip Socs won't
>> + * support non-aligned-to-DWORD transfer.
>> + */
>> + dwords = (len + 3) >> 2;
>
> Kernel has macros for rounding up, like DIV_ROUND_UP().
Sure.
>
>> + while (dwords) {
>> + tx_wl = (readl_relaxed(sfc->regbase + SFC_FSR) >>
>> + SFC_FSR_TX_WATER_LVL_SHIFT) &
>> + SFC_FSR_TX_WATER_LVL_MASK;
>> +
>> + if (tx_wl > 0) {
>> + count = min_t(u32, dwords, tx_wl);
>> + for (i = 0; i < count; i++) {
>> + writel_relaxed(*tbuf++,
>> + sfc->regbase + SFC_DATA);
>> + dwords--;
>> + }
>> +
>> + if (dwords == 0)
>> + break;
>> + timeout = 0;
>> + } else {
>> + mdelay(1);
>
> That is a long delay, shouldn't you wait using udelay() here ?
I will drop all these open coding stuff and use
{readl,writel}_poll_timeout API instead.
>
>> + if (timeout++ > SFC_MAX_IDLE_RETRY) {
>> + ret = -ETIMEDOUT;
>> + break;
>> + }
>> + }
>> + }
>> +
>> + if (ret)
>> + return ret;
>> + else
>> + return rockchip_sfc_wait_op_finish(sfc);
>> +}
>> +
>> +static inline int rockchip_sfc_pio_read(struct rockchip_sfc *sfc, u_char *buf,
>> + size_t len)
>> +{
>> + u32 dwords, rx_wl, count, i, tmp;
>> + unsigned long timeout;
>> + int ret = 0;
>> + u32 *tbuf = (u32 *)buf;
>> + u_char *tbuf2;
>> +
>> + /*
>> + * Align bytes to dwords, and get the remained bytes.
>> + * We should always round down to DWORD as the ahb for
>> + * Rockchip Socs won't support non-aligned-to-DWORD transfer.
>> + * So please don't use any APIs that will finally use non-aligned
>> + * read, for instance, memcpy_fromio or ioread32_rep etc.
>> + */
>> + dwords = len >> 2;
>> + len = len & 0x3;
>
> Won't this overwrite some bits past the $buf if you write more than $len
> bytes into this memory location ?
>
I can't see the possibility here to overwrite $buf, no?
>> + while (dwords) {
>> + rx_wl = (readl_relaxed(sfc->regbase + SFC_FSR) >>
>> + SFC_FSR_RX_WATER_LVL_SHIFT) &
>> + SFC_FSR_RX_WATER_LVL_MASK;
>> +
>> + if (rx_wl > 0) {
>> + count = min_t(u32, dwords, rx_wl);
>> + for (i = 0; i < count; i++) {
>> + *tbuf++ = readl_relaxed(sfc->regbase +
>> + SFC_DATA);
>> + dwords--;
>> + }
>> +
>> + if (dwords == 0)
>> + break;
>> + timeout = 0;
>> + } else {
>> + mdelay(1);
>> + if (timeout++ > SFC_MAX_IDLE_RETRY) {
>> + ret = -ETIMEDOUT;
>> + break;
>> + }
>> + }
>> + }
>> +
>> + if (ret)
>> + return ret;
>> +
>> + /* Read the remained bytes */
>> + timeout = 0;
>> + tbuf2 = (u_char *)tbuf;
>> + while (len) {
>> + rx_wl = (readl_relaxed(sfc->regbase + SFC_FSR) >>
>> + SFC_FSR_RX_WATER_LVL_SHIFT) &
>> + SFC_FSR_RX_WATER_LVL_MASK;
>> + if (rx_wl > 0) {
>> + tmp = readl_relaxed(sfc->regbase + SFC_DATA);
>> + for (i = 0; i < len; i++)
>> + tbuf2[i] = (u8)((tmp >> (i * 8)) & 0xff);
>> + goto done;
>> + } else {
>> + mdelay(1);
>> + if (timeout++ > SFC_MAX_IDLE_RETRY) {
>> + ret = -ETIMEDOUT;
>> + break;
>> + }
>> + }
>> + }
>
> Seems a lot like the write path, can you unify the code ?
yes, will drop all thease as mentioned above.
>
>> +done:
>> + if (ret)
>> + return ret;
>> + else
>> + return rockchip_sfc_wait_op_finish(sfc);
>> +}
>> +
>> +static int rockchip_sfc_pio_transfer(struct spi_nor *nor, loff_t from_to,
>> + size_t len, u_char *buf, u8 op_type)
>> +{
>> + struct rockchip_sfc_chip_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> +
>> + rockchip_sfc_setup_transfer(nor, from_to, len, op_type);
>> +
>> + if (op_type == SFC_CMD_DIR_WR)
>> + return rockchip_sfc_pio_write(sfc, buf, len);
>> + else
>> + return rockchip_sfc_pio_read(sfc, buf, len);
>> +}
>> +
>> +static ssize_t rockchip_sfc_read(struct spi_nor *nor, loff_t from, size_t len,
>> + u_char *read_buf)
>> +{
>> + struct rockchip_sfc_chip_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> + size_t offset;
>> + int ret;
>> + dma_addr_t dma_addr = 0;
>> +
>> + if (!sfc->use_dma)
>> + goto no_dma;
>
> You should extract this DMA code into rockchip_sfc_dma_read/write()
> instead and have this method-agnostic function only do the decision
> between calling the PIO one and DMA one. That'd improve the structure
> of the code a lot.
Sure.
>
>> + for (offset = 0; offset < len; offset += SFC_DMA_MAX_LEN) {
>> + size_t trans = min_t(size_t, SFC_DMA_MAX_LEN, len - offset);
>> +
>> + dma_addr = dma_map_single(NULL, (void *)read_buf,
>> + trans, DMA_FROM_DEVICE);
>> + if (dma_mapping_error(sfc->dev, dma_addr))
>> + dma_addr = 0;
>> +
>> + /* Fail to map dma, use pre-allocated area instead */
>> + ret = rockchip_sfc_dma_transfer(nor, from + offset,
>> + dma_addr ? dma_addr :
>> + sfc->dma_buffer,
>> + trans, SFC_CMD_DIR_RD);
>> +
>> + if (dma_addr) {
>> + /* Invalidate the read data from dma_addr */
>> + dma_sync_single_for_cpu(sfc->dev, dma_addr,
>> + trans, DMA_FROM_DEVICE);
>> + dma_unmap_single(NULL, dma_addr,
>> + trans, DMA_FROM_DEVICE);
>> + }
>> +
>> + if (ret) {
>> + dev_warn(nor->dev, "DMA read timeout\n");
>> + return ret;
>> + }
>> + if (!dma_addr)
>> + memcpy(read_buf + offset, sfc->buffer, trans);
>> + }
>> +
>> + return len;
>> +
>> +no_dma:
>> + ret = rockchip_sfc_pio_transfer(nor, from, len,
>> + read_buf, SFC_CMD_DIR_RD);
>> + if (ret) {
>> + dev_warn(nor->dev, "PIO read timeout\n");
>> + return ret;
>> + }
>> + return len;
>> +}
>> +
>> +static ssize_t rockchip_sfc_write(struct spi_nor *nor, loff_t to,
>> + size_t len, const u_char *write_buf)
>> +{
>> + struct rockchip_sfc_chip_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> + size_t offset;
>> + int ret;
>> + dma_addr_t dma_addr = 0;
>> +
>> + if (!sfc->use_dma)
>> + goto no_dma;
>> +
>> + for (offset = 0; offset < len; offset += SFC_DMA_MAX_LEN) {
>> + size_t trans = min_t(size_t, SFC_DMA_MAX_LEN, len - offset);
>> +
>> + dma_addr = dma_map_single(NULL, (void *)write_buf,
>> + trans, DMA_TO_DEVICE);
>> + if (dma_mapping_error(sfc->dev, dma_addr)) {
>> + dma_addr = 0;
>> + memcpy(sfc->buffer, write_buf + offset, trans);
>> + } else {
>> + /* Flush the write data to dma memory */
>> + dma_sync_single_for_device(sfc->dev, dma_addr,
>> + trans, DMA_TO_DEVICE);
>> + }
>> +
>> + /* Fail to map dma, use pre-allocated area instead */
>> + ret = rockchip_sfc_dma_transfer(nor, to + offset,
>> + dma_addr ? dma_addr :
>> + sfc->dma_buffer,
>> + trans, SFC_CMD_DIR_WR);
>> + if (dma_addr)
>> + dma_unmap_single(NULL, dma_addr,
>> + trans, DMA_TO_DEVICE);
>> + if (ret) {
>> + dev_warn(nor->dev, "DMA write timeout\n");
>> + return ret;
>> + }
>> + }
>
> Again, the read and write looks really similar. I wonder if you cannot
> parametrize it and unify the code.
>
okay. I will refacter them to unify the code.
>> + return len;
>> +no_dma:
>> + ret = rockchip_sfc_pio_transfer(nor, to, len,
>> + (u_char *)write_buf, SFC_CMD_DIR_WR);
>> + if (ret) {
>> + dev_warn(nor->dev, "PIO write timeout\n");
>> + return ret;
>> + }
>> + return len;
>> +}
>> +
>> +/**
>> + * Get spi flash device information and register it as a mtd device.
>> + */
>> +static int rockchip_sfc_register(struct device_node *np,
>> + struct rockchip_sfc *sfc)
>> +{
>> + struct device *dev = sfc->dev;
>> + struct mtd_info *mtd;
>> + int ret;
>> +
>> + sfc->flash[sfc->num_chip].nor.dev = dev;
>> + spi_nor_set_flash_node(&(sfc->flash[sfc->num_chip].nor), np);
>> +
>> + ret = of_property_read_u8(np, "reg", &sfc->flash[sfc->num_chip].cs);
>> + if (ret) {
>> + dev_err(dev, "No reg property for %s\n",
>> + np->full_name);
>> + return ret;
>> + }
>> +
>> + ret = of_property_read_u32(np, "spi-max-frequency",
>> + &sfc->flash[sfc->num_chip].clk_rate);
>> + if (ret) {
>> + dev_err(dev, "No spi-max-frequency property for %s\n",
>> + np->full_name);
>> + return ret;
>> + }
>> +
>> + sfc->flash[sfc->num_chip].sfc = sfc;
>> + sfc->flash[sfc->num_chip].nor.priv = &(sfc->flash[sfc->num_chip]);
>
> You can add nor = sfc->flash[sfc->num_chip].nor; here to avoid
> constantly replicating the whole sfc->flash[sfc->num_chip].nor .
Sure.
>
>> + sfc->flash[sfc->num_chip].nor.prepare = rockchip_sfc_prep;
>> + sfc->flash[sfc->num_chip].nor.unprepare = rockchip_sfc_unprep;
>> + sfc->flash[sfc->num_chip].nor.read_reg = rockchip_sfc_read_reg;
>> + sfc->flash[sfc->num_chip].nor.write_reg = rockchip_sfc_write_reg;
>> + sfc->flash[sfc->num_chip].nor.read = rockchip_sfc_read;
>> + sfc->flash[sfc->num_chip].nor.write = rockchip_sfc_write;
>> + sfc->flash[sfc->num_chip].nor.erase = NULL;
>> + ret = spi_nor_scan(&(sfc->flash[sfc->num_chip].nor),
>> + NULL, SPI_NOR_QUAD);
>> + if (ret)
>> + return ret;
>> +
>> + mtd = &(sfc->flash[sfc->num_chip].nor.mtd);
>> + mtd->name = np->name;
>> + ret = mtd_device_register(mtd, NULL, 0);
>> + if (ret)
>> + return ret;
>> +
>> + sfc->num_chip++;
>> + return 0;
>> +}
>> +
>> +static void rockchip_sfc_unregister_all(struct rockchip_sfc *sfc)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < sfc->num_chip; i++)
>> + mtd_device_unregister(&(sfc->flash[sfc->num_chip].nor.mtd));
>
> Same here. Also, what happens if you have a hole in the SPI NOR
> numbering, ie you have only SPI NOR 0 and 2 registered ? This will fail,
> so see the cadence qspi how to handle such case.
>
>> +}
>> +
>> +static int rockchip_sfc_register_all(struct rockchip_sfc *sfc)
>> +{
>> + struct device *dev = sfc->dev;
>> + struct device_node *np;
>> + int ret;
>> +
>> + for_each_available_child_of_node(dev->of_node, np) {
>> + ret = rockchip_sfc_register(np, sfc);
>> + if (ret)
>> + goto fail;
>> +
>> + if (sfc->num_chip == SFC_MAX_CHIPSELECT_NUM) {
>> + dev_warn(dev, "Exceeds the max cs limitation\n");
>> + break;
>> + }
>> + }
>> +
>> + return 0;
>> +
>> +fail:
>> + dev_err(dev, "Failed to register all chips\n");
>> + /* Unregister all the _registered_ nor flash */
>> + rockchip_sfc_unregister_all(sfc);
>> + return ret;
>> +}
>
> [...]
>
>> +static int rockchip_sfc_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct resource *res;
>> + struct rockchip_sfc *sfc;
>> + int ret;
>> +
>> + sfc = devm_kzalloc(dev, sizeof(*sfc), GFP_KERNEL);
>> + if (!sfc)
>> + return -ENOMEM;
>> +
>> + platform_set_drvdata(pdev, sfc);
>> + sfc->dev = dev;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + sfc->regbase = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(sfc->regbase))
>> + return PTR_ERR(sfc->regbase);
>> +
>> + sfc->clk = devm_clk_get(&pdev->dev, "sfc");
>> + if (IS_ERR(sfc->clk)) {
>> + dev_err(&pdev->dev, "Failed to get sfc interface clk\n");
>> + return PTR_ERR(sfc->clk);
>> + }
>> +
>> + sfc->hclk = devm_clk_get(&pdev->dev, "hsfc");
>> + if (IS_ERR(sfc->hclk)) {
>> + dev_err(&pdev->dev, "Failed to get sfc ahp clk\n");
>> + return PTR_ERR(sfc->hclk);
>> + }
>> +
>> + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>> + if (ret) {
>> + dev_warn(dev, "Unable to set dma mask\n");
>> + return ret;
>> + }
>> +
>> + sfc->buffer = dmam_alloc_coherent(dev, SFC_DMA_MAX_LEN,
>> + &sfc->dma_buffer, GFP_KERNEL);
>> + if (!sfc->buffer)
>> + return -ENOMEM;
>> +
>> + mutex_init(&sfc->lock);
>> +
>> + ret = clk_prepare_enable(sfc->hclk);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to enable hclk\n");
>> + goto err_hclk;
>> + }
>> +
>> + ret = clk_prepare_enable(sfc->clk);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to enable clk\n");
>> + goto err_clk;
>> + }
>> +
>> + sfc->use_dma = !of_property_read_bool(sfc->dev->of_node,
>> + "rockchip,sfc-no-dma");
>> +
>> + sfc->negative_edge = of_device_is_compatible(sfc->dev->of_node,
>> + "rockchip,rk1108-sfc");
>
> I think this should rather be a boolean property -- but isn't this
> something like CPOL or CPHA anyway ?
It isn't the same as CPOL or CPHA. And this value should be Soc
specificed.
>
>> + /* Find the irq */
>> + ret = platform_get_irq(pdev, 0);
>> + if (ret < 0) {
>> + dev_err(dev, "Failed to get the irq\n");
>> + goto err_irq;
>> + }
>> +
>> + ret = devm_request_irq(dev, ret, rockchip_sfc_irq_handler,
>> + 0, pdev->name, sfc);
>> + if (ret) {
>> + dev_err(dev, "Failed to request irq\n");
>> + goto err_irq;
>> + }
>> +
>> + sfc->num_chip = 0;
>> + ret = rockchip_sfc_init(sfc);
>> + if (ret)
>> + goto err_irq;
>> +#if 1
>> + pm_runtime_get_noresume(&pdev->dev);
>> + pm_runtime_set_active(&pdev->dev);
>> + pm_runtime_enable(&pdev->dev);
>> + pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
>> + pm_runtime_use_autosuspend(&pdev->dev);
>> +#endif
>
> #if 1, remove, #endif :-)
Ah, will fix.
>
>> + ret = rockchip_sfc_register_all(sfc);
>> + if (ret)
>> + goto err_register;
>> +
>> + clk_disable_unprepare(sfc->clk);
>> + pm_runtime_put_autosuspend(&pdev->dev);
>> + return 0;
>> +
>> +err_register:
>> + pm_runtime_disable(&pdev->dev);
>> + pm_runtime_set_suspended(&pdev->dev);
>> + pm_runtime_put_noidle(&pdev->dev);
>> +err_irq:
>> + clk_disable_unprepare(sfc->clk);
>> +err_clk:
>> + clk_disable_unprepare(sfc->hclk);
>> +err_hclk:
>> + mutex_destroy(&sfc->lock);
>> + return ret;
>> +}
>> +
>> +static int rockchip_sfc_remove(struct platform_device *pdev)
>> +{
>> + struct rockchip_sfc *sfc = platform_get_drvdata(pdev);
>> +
>> + pm_runtime_get_sync(&pdev->dev);
>> + pm_runtime_disable(&pdev->dev);
>> + pm_runtime_put_noidle(&pdev->dev);
>> +
>> + rockchip_sfc_unregister_all(sfc);
>> + mutex_destroy(&sfc->lock);
>> + clk_disable_unprepare(sfc->clk);
>> + clk_disable_unprepare(sfc->hclk);
>> + return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +int rockchip_sfc_runtime_suspend(struct device *dev)
>> +{
>> + struct rockchip_sfc *sfc = dev_get_drvdata(dev);
>> +
>> + clk_disable_unprepare(sfc->hclk);
>> + return 0;
>> +}
>> +
>> +int rockchip_sfc_runtime_resume(struct device *dev)
>> +{
>> + struct rockchip_sfc *sfc = dev_get_drvdata(dev);
>> +
>> + clk_prepare_enable(sfc->hclk);
>> + return 0;
>> +}
>> +#endif /* CONFIG_PM */
>> +
>> +static const struct of_device_id rockchip_sfc_dt_ids[] = {
>> + { .compatible = "rockchip,sfc"},
>> + { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, rockchip_sfc_dt_ids);
>> +
>> +static const struct dev_pm_ops rockchip_sfc_dev_pm_ops = {
>> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>> + pm_runtime_force_resume)
>> + SET_RUNTIME_PM_OPS(rockchip_sfc_runtime_suspend,
>> + rockchip_sfc_runtime_resume, NULL)
>> +};
>> +
>> +static struct platform_driver rockchip_sfc_driver = {
>> + .driver = {
>> + .name = "rockchip-sfc",
>> + .of_match_table = rockchip_sfc_dt_ids,
>> + .pm = &rockchip_sfc_dev_pm_ops,
>> + },
>> + .probe = rockchip_sfc_probe,
>> + .remove = rockchip_sfc_remove,
>> +};
>> +module_platform_driver(rockchip_sfc_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("Rockchip Serial Flash Controller Driver");
>> +MODULE_AUTHOR("Shawn Lin");
>
> Email in MODULE_AUTHOR would be great addition.
yup.
>
--
Best Regards
Shawn Lin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] mmc: pwrseq: add support for Marvell SD8787 chip
From: Matt Ranostay @ 2016-11-30 1:20 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Linux Kernel, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Tony Lindgren,
Ulf Hansson, Mark Rutland, Srinivas Kandagatla
In-Reply-To: <CABxcv=nEih8xX4fM0eqjkqprdt1uGTEyx5tzK04GuSgsHc=Haw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tue, Nov 29, 2016 at 9:13 AM, Javier Martinez Canillas
<javier-0uQlZySMnqxg9hUCZPvPmw@public.gmane.org> wrote:
> Hello Matt,
>
> On Thu, Nov 17, 2016 at 10:55 PM, Matt Ranostay
> <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org> wrote:
>> Allow power sequencing for the Marvell SD8787 Wifi/BT chip.
>> This can be abstracted to other chipsets if needed in the future.
>>
>> Cc: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
>> Cc: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
>> Cc: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
>> ---
>> .../devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt | 14 +++
>> .../bindings/net/wireless/marvell-sd8xxx.txt | 4 +
>> drivers/mmc/core/Kconfig | 10 ++
>> drivers/mmc/core/Makefile | 1 +
>> drivers/mmc/core/pwrseq_sd8787.c | 117 +++++++++++++++++++++
>> 5 files changed, 146 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt
>> create mode 100644 drivers/mmc/core/pwrseq_sd8787.c
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt
>
> According Documentation/devicetree/bindings/submitting-patches.txt,
> the DT bindings patches should posted as a separate patch.
Ok will do.
>
>> new file mode 100644
>> index 000000000000..1b658351629b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt
>> @@ -0,0 +1,14 @@
>> +* Marvell SD8787 power sequence provider
>> +
>> +Required properties:
>> +- compatible: must be "mmc-pwrseq-sd8787".
>
> Since this is not a generic binding, the compatible string should have
> a vendor prefix.
>
Makes sense to me.
>> +- pwndn-gpio: contains a power down GPIO specifier.
>> +- reset-gpio: contains a reset GPIO specifier.
>> +
>
> I wonder if we really need a custom power sequence provider for just
> this SDIO WiFI chip though. AFAICT the only missing piece in
> mmc-pwrseq-simple is the power down GPIO property, so maybe
> mmc-pwrseq-simple could be extended instead to have an optional
> powerdown-gpios property and instead in the Marvell SD8787 DT binding
> can be mentioned which mmc-pwrseq-simple properties are required for
> the device.
>
The reason we didn't do that is we need delay between the two
assertions/desertions of GPIOs. It wouldn't seems good practice to
hack the pwrseq-simple for this...
>> +Example:
>> +
>> + wifi_pwrseq: wifi_pwrseq {
>> + compatible = "mmc-pwrseq-sd8787";
>> + pwrdn-gpio = <&twl_gpio 0 GPIO_ACTIVE_LOW>;
>> + reset-gpio = <&twl_gpio 1 GPIO_ACTIVE_LOW>;
>> + }
>> diff --git a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>
> Does this patch depend on a previous posted series? I don't see this
> file in today's linux-next...
Got renamed to ->
Documentation/devicetree/bindings/net/wireless/marvell-8xxx.txt it
seems very recently.
>
>> index c421aba0a5bc..08fd65d35725 100644
>> --- a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>> +++ b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>> @@ -32,6 +32,9 @@ Optional properties:
>> so that the wifi chip can wakeup host platform under certain condition.
>> during system resume, the irq will be disabled to make sure
>> unnecessary interrupt is not received.
>> + - vmmc-supply: a phandle of a regulator, supplying VCC to the card
>> + - mmc-pwrseq: phandle to the MMC power sequence node. See "mmc-pwrseq-*"
>> + for documentation of MMC power sequence bindings.
>>
>> Example:
>>
>> @@ -44,6 +47,7 @@ so that firmware can wakeup host using this device side pin.
>> &mmc3 {
>> status = "okay";
>> vmmc-supply = <&wlan_en_reg>;
>> + mmc-pwrseq = <&wifi_pwrseq>;
>> bus-width = <4>;
>> cap-power-off-card;
>> keep-power-in-suspend;
>
> I think this change should be split in a separate patch as well.
>
> Best regards,
> Javier
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v10 3/4] dtc: Plugin and fixup support
From: David Gibson @ 2016-11-30 1:41 UTC (permalink / raw)
To: Phil Elwell
Cc: Pantelis Antoniou, Jon Loeliger, Grant Likely, Frank Rowand,
Rob Herring, Jan Luebbe, Sascha Hauer, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <b7ff53f6-6481-e3f1-e3b5-d0b04e563e83-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2625 bytes --]
On Tue, Nov 29, 2016 at 10:32:28AM +0000, Phil Elwell wrote:
> On 29/11/2016 02:11, David Gibson wrote:
> > On Mon, Nov 28, 2016 at 12:24:20PM +0000, Phil Elwell wrote:
> >> On 28/11/2016 12:10, Pantelis Antoniou wrote:
> >>> For plugins we need the __symbols__ node to support stacked overlays, i.e.
> >>> overlays referring label that were introduced by a previous overlay.
> >> Although it is arguably useful to be able to refer to symbols created by
> >> one overlay from within another, do we really want all symbols to be
> >> global? Isn't there a call for a new syntax or usage pattern to indicate
> >> either that a symbol should be local to the overlay or, my preferred
> >> option, global?
> > So, this is back to a design question about the overlay format. As
> > noted in the initial discussions about possible "connector" formats, I
> > think we will want some sort of local symbols. But the current
> > overlay format with all global symbols is out there and we need to
> > support it.
> The overlay format we have does not dictate the scope of the symbols.
>
> In all implementations I know of - the Raspberry Pi loader, the current
> Linux kernel, the latest dtc patch set - there is a completely
> asymmetric relationship between the base DTB and an overlay:
> * the base DTB exports __symbols__ to resolve the overlays unresolved
> label references, as recorded by the __fixups__ node
> * the overlay's phandles are renumbered so as not to clash with the base
> tree using the __local_fixups__
> * the contents of the __overlay__ nodes are applied to the base tree, as
> directed by the "target" or "target-path" properties
>
> The __symbols__ node of the overlay is ignored and discarded. The
> __fixups__ and __local_fixups__ in the base DTB (if present - the RPi
> dtc only generates them for /plugins/) are ignored.
>
> In the set of RPi overlays only one exports a global symbol, which it
> achieves with an overlay aimed at target-path = "/__symbols__" that adds
> a new symbol (in this case "i2c_gpio").
>
> If the __symbols__ in an overlay are automatically merged with the base
> symbols, that is a significant change in semantics which needs to be
> discussed.
Symbols are global, they really can't be anything else in this format.
This is why I think the connector approach - though substantially more
complex - is a better idea in the wrong run.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v10 3/4] dtc: Plugin and fixup support
From: David Gibson @ 2016-11-30 1:42 UTC (permalink / raw)
To: Phil Elwell
Cc: Pantelis Antoniou, Jon Loeliger, Grant Likely, Frank Rowand,
Rob Herring, Jan Luebbe, Sascha Hauer, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <66c7f8c5-94e9-a6ca-4402-fa0ccf2a6ac0-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 3404 bytes --]
On Tue, Nov 29, 2016 at 10:50:53AM +0000, Phil Elwell wrote:
> On 29/11/2016 10:39, Pantelis Antoniou wrote:
> > Hi Phil,
> >
> >> On Nov 29, 2016, at 12:32 , Phil Elwell <phil-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org> wrote:
> >>
> >> On 29/11/2016 02:11, David Gibson wrote:
> >>> On Mon, Nov 28, 2016 at 12:24:20PM +0000, Phil Elwell wrote:
> >>>> On 28/11/2016 12:10, Pantelis Antoniou wrote:
> >>>>> For plugins we need the __symbols__ node to support stacked overlays, i.e.
> >>>>> overlays referring label that were introduced by a previous overlay.
> >>>> Although it is arguably useful to be able to refer to symbols created by
> >>>> one overlay from within another, do we really want all symbols to be
> >>>> global? Isn't there a call for a new syntax or usage pattern to indicate
> >>>> either that a symbol should be local to the overlay or, my preferred
> >>>> option, global?
> >>> So, this is back to a design question about the overlay format. As
> >>> noted in the initial discussions about possible "connector" formats, I
> >>> think we will want some sort of local symbols. But the current
> >>> overlay format with all global symbols is out there and we need to
> >>> support it.
> >> The overlay format we have does not dictate the scope of the symbols.
> >>
> >> In all implementations I know of - the Raspberry Pi loader, the current
> >> Linux kernel, the latest dtc patch set - there is a completely
> >> asymmetric relationship between the base DTB and an overlay:
> >> * the base DTB exports __symbols__ to resolve the overlays unresolved
> >> label references, as recorded by the __fixups__ node
> >> * the overlay's phandles are renumbered so as not to clash with the base
> >> tree using the __local_fixups__
> >> * the contents of the __overlay__ nodes are applied to the base tree, as
> >> directed by the "target" or "target-path” properties
> >>
> > Yes
> >
> >> The __symbols__ node of the overlay is ignored and discarded. The
> >> __fixups__ and __local_fixups__ in the base DTB (if present - the RPi
> >> dtc only generates them for /plugins/) are ignored.
> >>
> > That was a limitation that no-longer applies. Overlay symbols can be added
> > to the base tree symbol list with a small patch I have already posted.
> The fact that they can doesn't mean they necessarily should.
They should - at least as long as we're using this simple overlay
format. What else would you do with them? If they're not merged,
they're useless in the overlay.
> >
> >> In the set of RPi overlays only one exports a global symbol, which it
> >> achieves with an overlay aimed at target-path = "/__symbols__" that adds
> >> a new symbol (in this case "i2c_gpio").
> >>
> >> If the __symbols__ in an overlay are automatically merged with the base
> >> symbols, that is a significant change in semantics which needs to be
> >> discussed.
> >>
> > You no longer need to do this anymore. Hope this helps :)
>
> Not really, no. With the current scheme we have control over the scope,
> but now there is none.
You really don't, symbols were always global.
>
> How does your patch handle duplicate symbols?
>
> Phil
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v10 3/4] dtc: Plugin and fixup support
From: David Gibson @ 2016-11-30 1:49 UTC (permalink / raw)
To: Phil Elwell
Cc: Pantelis Antoniou, Jon Loeliger, Grant Likely, Frank Rowand,
Rob Herring, Jan Luebbe, Sascha Hauer, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <ba8e2ed3-9798-3074-1167-3f6851321a25-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 3560 bytes --]
On Tue, Nov 29, 2016 at 12:11:15PM +0000, Phil Elwell wrote:
> Pantelis,
>
> On 29/11/2016 10:55, Pantelis Antoniou wrote:
> > Manually adding symbols by targeting __symbols__ is just bad. There is absolutely
> > no guarantee that the symbol/fixup node(s) will still be there in following iterations
> > of the patches.
> Remember that this is now part of the Linux kernel - it isn't something
> you can just change at will.
> > I am thinking of parsing them, recording the information in kernel structures and then
> > deleting them altogether.
> >
> >> How does your patch handle duplicate symbols?
> >>
> > It doesn’t. Having duplicate global symbols is bad.
> >
> > It appears you want scoping rules instead. Care to paste a concrete example?
>
> Concrete non-trivial examples are hard to come by. There are some simple
> cases where we've attached labels to __overlay__ nodes so that the
> contents can be patched by our overlay parameter mechanism - they could
> just be given unique names instead of just "frag0", "frag1" etc. I'm
> more concerned about parameterised macro-expanded overlays.
>
> Consider an overlay that defines a CAN controller on an SPI bus. We
> currently have two such overlays in the RPi tree, one for SPI 0.0 and
> one for SPI 0.1. Here's an extract from one of them:
>
> /* the interrupt pin of the can-controller */
> fragment@2 {
> target = <&gpio>;
> __overlay__ {
> can0_pins: can0_pins {
> brcm,pins = <25>;
> brcm,function = <0>; /* input */
> };
> };
> };
> ...
> fragment@4 {
> target = <&spi0>;
> __overlay__ {
> /* needed to avoid dtc warning */
> #address-cells = <1>;
> #size-cells = <0>;
> can0: mcp2515@0 {
> reg = <0>;
> compatible = "microchip,mcp2515";
> pinctrl-names = "default";
> pinctrl-0 = <&can0_pins>;
> spi-max-frequency = <10000000>;
> interrupt-parent = <&gpio>;
> interrupts = <25 0x2>;
> clocks = <&can0_osc>;
> };
> };
> };
>
> One day I'd like to merge these into a single parameterised version that
> could target any CS line on any SPI controller. This requires that any
> created node names are unique with the scope of the parent ("mcp2515@0",
> "can0_pins"), and that the name of the target label (spi0) is patched to
> select the correct SPI bus. Our existing, limited overlay parameter
> mechanism uses labels to identify properties to patch:
Yeah, I think this is basically out of scope for the simple overlay
format. It's really not designed to do this - it works directly with
the global namespace everywhere.
The connector proposals are designed to address this sort of case.
This sort of limitation is, incidentally, why I objected to the dtb
overlay stuff when it was originally proposed (I would have preferred
something closer to the connector proposals which are now floating
again). That in turn is why it's taken so long to get on its way into
mainline dtc. But despite the limitations, people wanted it badly
enough that it's become widely used, so now we have to cope with it.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v10 3/4] dtc: Plugin and fixup support
From: David Gibson @ 2016-11-30 1:50 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Jon Loeliger, Grant Likely, Frank Rowand, Rob Herring, Jan Luebbe,
Sascha Hauer, Phil Elwell, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CC3401F7-9DE7-4913-8FE6-DB1E89E20A3A-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 15036 bytes --]
On Tue, Nov 29, 2016 at 01:09:11PM +0200, Pantelis Antoniou wrote:
> Hi David,
>
> > On Nov 29, 2016, at 04:10 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Mon, Nov 28, 2016 at 02:10:35PM +0200, Pantelis Antoniou wrote:
> >>
> >>> On Nov 28, 2016, at 06:12 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >>>
> >>> On Fri, Nov 25, 2016 at 02:32:10PM +0200, Pantelis Antoniou wrote:
> >>>> This patch enable the generation of symbols & local fixup information
> >>>> for trees compiled with the -@ (--symbols) option.
> >>>>
> >>>> Using this patch labels in the tree and their users emit information
> >>>> in __symbols__ and __local_fixups__ nodes.
> >>>>
> >>>> The __fixups__ node make possible the dynamic resolution of phandle
> >>>> references which are present in the plugin tree but lie in the
> >>>> tree that are applying the overlay against.
> >>>>
> >>>> While there is a new magic number for dynamic device tree/overlays blobs
> >>>> it is by default enabled. Remember to use -M to generate compatible
> >>>> blobs.
> >>>>
> >>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> >>>> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >>>> Signed-off-by: Jan Luebbe <jlu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >>>> ---
> >>>> Documentation/manual.txt | 25 +++++-
> >>>> checks.c | 8 +-
> >>>> dtc-lexer.l | 5 ++
> >>>> dtc-parser.y | 50 +++++++++--
> >>>> dtc.c | 39 +++++++-
> >>>> dtc.h | 20 ++++-
> >>>> fdtdump.c | 2 +-
> >>>> flattree.c | 17 ++--
> >>>> fstree.c | 2 +-
> >>>> libfdt/fdt.c | 2 +-
> >>>> libfdt/fdt.h | 3 +-
> >>>> livetree.c | 225 ++++++++++++++++++++++++++++++++++++++++++++++-
> >>>> tests/mangle-layout.c | 7 +-
> >>>> 13 files changed, 375 insertions(+), 30 deletions(-)
> >>>>
> >>>> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
> >>>> index 398de32..094893b 100644
> >>>> --- a/Documentation/manual.txt
> >>>> +++ b/Documentation/manual.txt
> >>>> @@ -119,6 +119,24 @@ Options:
> >>>> Make space for <number> reserve map entries
> >>>> Relevant for dtb and asm output only.
> >>>>
> >>>> + -@
> >>>> + Generates a __symbols__ node at the root node of the resulting blob
> >>>> + for any node labels used, and for any local references using phandles
> >>>> + it also generates a __local_fixups__ node that tracks them.
> >>>> +
> >>>> + When using the /plugin/ tag all unresolved label references to
> >>>> + be tracked in the __fixups__ node, making dynamic resolution possible.
> >>>> +
> >>>> + -A
> >>>> + Generate automatically aliases for all node labels. This is similar to
> >>>> + the -@ option (the __symbols__ node contain identical information) but
> >>>> + the semantics are slightly different since no phandles are automatically
> >>>> + generated for labeled nodes.
> >>>> +
> >>>> + -M
> >>>> + Generate blobs with the old FDT magic number for device tree objects.
> >>>> + By default blobs use the DTBO FDT magic number instead.
> >>>> +
> >>>> -S <bytes>
> >>>> Ensure the blob at least <bytes> long, adding additional
> >>>> space if needed.
> >>>> @@ -146,13 +164,18 @@ Additionally, dtc performs various sanity checks on the tree.
> >>>> Here is a very rough overview of the layout of a DTS source file:
> >>>>
> >>>>
> >>>> - sourcefile: list_of_memreserve devicetree
> >>>> + sourcefile: versioninfo plugindecl list_of_memreserve devicetree
> >>>>
> >>>> memreserve: label 'memreserve' ADDR ADDR ';'
> >>>> | label 'memreserve' ADDR '-' ADDR ';'
> >>>>
> >>>> devicetree: '/' nodedef
> >>>>
> >>>> + versioninfo: '/' 'dts-v1' '/' ';'
> >>>> +
> >>>> + plugindecl: '/' 'plugin' '/' ';'
> >>>> + | /* empty */
> >>>> +
> >>>> nodedef: '{' list_of_property list_of_subnode '}' ';'
> >>>>
> >>>> property: label PROPNAME '=' propdata ';'
> >>>> diff --git a/checks.c b/checks.c
> >>>> index 2bd27a4..4292f4b 100644
> >>>> --- a/checks.c
> >>>> +++ b/checks.c
> >>>> @@ -487,8 +487,12 @@ static void fixup_phandle_references(struct check *c, struct boot_info *bi,
> >>>>
> >>>> refnode = get_node_by_ref(dt, m->ref);
> >>>> if (! refnode) {
> >>>> - FAIL(c, "Reference to non-existent node or label \"%s\"\n",
> >>>> - m->ref);
> >>>> + if (!(bi->versionflags & VF_PLUGIN))
> >>>> + FAIL(c, "Reference to non-existent node or "
> >>>> + "label \"%s\"\n", m->ref);
> >>>> + else /* mark the entry as unresolved */
> >>>> + *((cell_t *)(prop->val.val + m->offset)) =
> >>>> + cpu_to_fdt32(0xffffffff);
> >>>> continue;
> >>>> }
> >>>>
> >>>> diff --git a/dtc-lexer.l b/dtc-lexer.l
> >>>> index 790fbf6..40bbc87 100644
> >>>> --- a/dtc-lexer.l
> >>>> +++ b/dtc-lexer.l
> >>>> @@ -121,6 +121,11 @@ static void lexical_error(const char *fmt, ...);
> >>>> return DT_V1;
> >>>> }
> >>>>
> >>>> +<*>"/plugin/" {
> >>>> + DPRINT("Keyword: /plugin/\n");
> >>>> + return DT_PLUGIN;
> >>>> + }
> >>>> +
> >>>> <*>"/memreserve/" {
> >>>> DPRINT("Keyword: /memreserve/\n");
> >>>> BEGIN_DEFAULT();
> >>>> diff --git a/dtc-parser.y b/dtc-parser.y
> >>>> index 14aaf2e..1a1f660 100644
> >>>> --- a/dtc-parser.y
> >>>> +++ b/dtc-parser.y
> >>>> @@ -19,6 +19,7 @@
> >>>> */
> >>>> %{
> >>>> #include <stdio.h>
> >>>> +#include <inttypes.h>
> >>>>
> >>>> #include "dtc.h"
> >>>> #include "srcpos.h"
> >>>> @@ -33,6 +34,7 @@ extern void yyerror(char const *s);
> >>>>
> >>>> extern struct boot_info *the_boot_info;
> >>>> extern bool treesource_error;
> >>>> +
> >>>
> >>> Extraneous whitespace change here
> >>>
> >>
> >> OK.
> >>
> >>>> %}
> >>>>
> >>>> %union {
> >>>> @@ -52,9 +54,11 @@ extern bool treesource_error;
> >>>> struct node *nodelist;
> >>>> struct reserve_info *re;
> >>>> uint64_t integer;
> >>>> + unsigned int flags;
> >>>> }
> >>>>
> >>>> %token DT_V1
> >>>> +%token DT_PLUGIN
> >>>> %token DT_MEMRESERVE
> >>>> %token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
> >>>> %token DT_BITS
> >>>> @@ -71,6 +75,8 @@ extern bool treesource_error;
> >>>>
> >>>> %type <data> propdata
> >>>> %type <data> propdataprefix
> >>>> +%type <flags> versioninfo
> >>>> +%type <flags> plugindecl
> >>>> %type <re> memreserve
> >>>> %type <re> memreserves
> >>>> %type <array> arrayprefix
> >>>> @@ -101,16 +107,34 @@ extern bool treesource_error;
> >>>> %%
> >>>>
> >>>> sourcefile:
> >>>> - v1tag memreserves devicetree
> >>>> + versioninfo plugindecl memreserves devicetree
> >>>> + {
> >>>> + the_boot_info = build_boot_info($1 | $2, $3, $4,
> >>>> + guess_boot_cpuid($4));
> >>>> + }
> >>>> + ;
> >>>> +
> >>>> +versioninfo:
> >>>> + v1tag
> >>>> {
> >>>> - the_boot_info = build_boot_info($2, $3,
> >>>> - guess_boot_cpuid($3));
> >>>> + $$ = VF_DT_V1;
> >>>> }
> >>>> ;
> >>>>
> >>>> v1tag:
> >>>> DT_V1 ';'
> >>>> + | DT_V1
> >>>> | DT_V1 ';' v1tag
> >>>> +
> >>>> +plugindecl:
> >>>> + DT_PLUGIN ';'
> >>>> + {
> >>>> + $$ = VF_PLUGIN;
> >>>> + }
> >>>> + | /* empty */
> >>>> + {
> >>>> + $$ = 0;
> >>>> + }
> >>>> ;
> >>>>
> >>>> memreserves:
> >>>> @@ -161,10 +185,19 @@ devicetree:
> >>>> {
> >>>> struct node *target = get_node_by_ref($1, $2);
> >>>>
> >>>> - if (target)
> >>>> + if (target) {
> >>>> merge_nodes(target, $3);
> >>>> - else
> >>>> - ERROR(&@2, "Label or path %s not found", $2);
> >>>> + } else {
> >>>> + /*
> >>>> + * We rely on the rule being always:
> >>>> + * versioninfo plugindecl memreserves devicetree
> >>>> + * so $-1 is what we want (plugindecl)
> >>>> + */
> >>>> + if ($<flags>-1 & VF_PLUGIN)
> >>>
> >>> o_O... ok. I've never seen negative value references before. Can you
> >>> provide a link to some documentation saying this is actually supported
> >>> usage in bison? I wasn't able to find it when I looked.
> >>>
> >>
> >> There is a section about inherited attributes in the flex & bison book by O’Reily.
> >>
> >> https://books.google.gr/books?id=3Sr1V5J9_qMC&lpg=PP1&dq=flex%20bison&hl=el&pg=PP1#v=onepage&q=flex%20bison&f=false
> >>
> >> There’s a direct link to the 2nd Edition of lex & yacc:
> >>
> >> https://books.google.gr/books?id=fMPxfWfe67EC&lpg=PA183&ots=RcRSji2NAT&dq=yacc%20inherited%20attributes&hl=el&pg=PA183#v=onepage&q=yacc%20inherited%20attributes&f=false
> >
> > Thanks for the link. I still think moving the fragment assembly out
> > of the parser will be a better idea long term, but this does address
> > the main concern I had, so it will do for now.
> >
> >>>> + add_orphan_node($1, $3, $2);
> >>>> + else
> >>>> + ERROR(&@2, "Label or path %s not found", $2);
> >>>> + }
> >>>> $$ = $1;
> >>>> }
> >>>> | devicetree DT_DEL_NODE DT_REF ';'
> >>>> @@ -179,6 +212,11 @@ devicetree:
> >>>>
> >>>> $$ = $1;
> >>>> }
> >>>> + | /* empty */
> >>>> + {
> >>>> + /* build empty node */
> >>>> + $$ = name_node(build_node(NULL, NULL), "");
> >>>> + }
> >>>> ;
> >>>>
> >>>> nodedef:
> >>>> diff --git a/dtc.c b/dtc.c
> >>>> index 9dcf640..06e91bc 100644
> >>>> --- a/dtc.c
> >>>> +++ b/dtc.c
> >>>> @@ -32,6 +32,9 @@ int minsize; /* Minimum blob size */
> >>>> int padsize; /* Additional padding to blob */
> >>>> int alignsize; /* Additional padding to blob accroding to the alignsize */
> >>>> int phandle_format = PHANDLE_BOTH; /* Use linux,phandle or phandle properties */
> >>>> +int symbol_fixup_support; /* enable symbols & fixup support */
> >>>> +int auto_label_aliases; /* auto generate labels -> aliases */
> >>>> +int no_dtbo_magic; /* use old FDT magic values for objects */
> >>>>
> >>>> static int is_power_of_2(int x)
> >>>> {
> >>>> @@ -59,7 +62,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
> >>>> #define FDT_VERSION(version) _FDT_VERSION(version)
> >>>> #define _FDT_VERSION(version) #version
> >>>> static const char usage_synopsis[] = "dtc [options] <input file>";
> >>>> -static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:hv";
> >>>> +static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:@AMhv";
> >>>> static struct option const usage_long_opts[] = {
> >>>> {"quiet", no_argument, NULL, 'q'},
> >>>> {"in-format", a_argument, NULL, 'I'},
> >>>> @@ -78,6 +81,9 @@ static struct option const usage_long_opts[] = {
> >>>> {"phandle", a_argument, NULL, 'H'},
> >>>> {"warning", a_argument, NULL, 'W'},
> >>>> {"error", a_argument, NULL, 'E'},
> >>>> + {"symbols", no_argument, NULL, '@'},
> >>>> + {"auto-alias", no_argument, NULL, 'A'},
> >>>> + {"no-dtbo-magic", no_argument, NULL, 'M'},
> >>>> {"help", no_argument, NULL, 'h'},
> >>>> {"version", no_argument, NULL, 'v'},
> >>>> {NULL, no_argument, NULL, 0x0},
> >>>> @@ -109,6 +115,9 @@ static const char * const usage_opts_help[] = {
> >>>> "\t\tboth - Both \"linux,phandle\" and \"phandle\" properties",
> >>>> "\n\tEnable/disable warnings (prefix with \"no-\")",
> >>>> "\n\tEnable/disable errors (prefix with \"no-\")",
> >>>> + "\n\tEnable symbols/fixup support",
> >>>> + "\n\tEnable auto-alias of labels",
> >>>> + "\n\tDo not use DTBO magic value for plugin objects",
> >>>> "\n\tPrint this help and exit",
> >>>> "\n\tPrint version and exit",
> >>>> NULL,
> >>>> @@ -153,7 +162,7 @@ static const char *guess_input_format(const char *fname, const char *fallback)
> >>>> fclose(f);
> >>>>
> >>>> magic = fdt32_to_cpu(magic);
> >>>> - if (magic == FDT_MAGIC)
> >>>> + if (magic == FDT_MAGIC || magic == FDT_MAGIC_DTBO)
> >>>> return "dtb";
> >>>>
> >>>> return guess_type_by_name(fname, fallback);
> >>>> @@ -172,6 +181,7 @@ int main(int argc, char *argv[])
> >>>> FILE *outf = NULL;
> >>>> int outversion = DEFAULT_FDT_VERSION;
> >>>> long long cmdline_boot_cpuid = -1;
> >>>> + fdt32_t out_magic = FDT_MAGIC;
> >>>>
> >>>> quiet = 0;
> >>>> reservenum = 0;
> >>>> @@ -249,6 +259,16 @@ int main(int argc, char *argv[])
> >>>> parse_checks_option(false, true, optarg);
> >>>> break;
> >>>>
> >>>> + case '@':
> >>>> + symbol_fixup_support = 1;
> >>>> + break;
> >>>> + case 'A':
> >>>> + auto_label_aliases = 1;
> >>>> + break;
> >>>> + case 'M':
> >>>> + no_dtbo_magic = 1;
> >>>> + break;
> >>>> +
> >>>> case 'h':
> >>>> usage(NULL);
> >>>> default:
> >>>> @@ -306,6 +326,14 @@ int main(int argc, char *argv[])
> >>>> fill_fullpaths(bi->dt, "");
> >>>> process_checks(force, bi);
> >>>>
> >>>> + if (auto_label_aliases)
> >>>> + generate_label_tree(bi->dt, "aliases", false);
> >>>> +
> >>>> + if (symbol_fixup_support) {
> >>>> + generate_label_tree(bi->dt, "__symbols__", true);
> >>>> + generate_fixups_tree(bi->dt);
> >>>
> >>> Hang on.. this doesn't seem right. I thought -@ controlled the
> >>> __symbols__ side (i.e. the part upon which we overlay) rather than the
> >>> fixups side (the part which overlays). A dtbo could certainly have
> >>> both, of course, but for base trees, wouldn't you have symbols without
> >>> fixups? And should it be illegal to try to build a /plugin/ without
> >>> -@?
> >>
> >> It does control both for now. For base trees having the fixup nodes
> >> will allow us to do probe order dependency tracking in the future.
> >
> > Erm.. how?
> >
> >> For plugins we need the __symbols__ node to support stacked overlays, i.e.
> >> overlays referring label that were introduced by a previous overlay.
> >
> > Yes, I realise that an overlay may well want __symbols__ as well. But
> > they still seem conceptually different. I think -@ should control
> > __symbols__ whereas /plugin/ should control __fixups__.
> >
>
> It is easily done. Although using /plugin/ as an auto-magic option does both
> just fine.
Sorry, I don't follow what you're saying.
> >> For plugins there is no requirement for now to actually contain references to
> >> be resolved. It can easily be enforced though.
> >
> > Sure, but I don't see the relevance of that here. You could just omit
> > the __fixups__ node if there's nothing to go into them.
> >
>
> Hmm, yeah.
>
>
> Regards
>
> — Pantelis
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 3/3] ARM: dts: sunxi: enable SDIO Wi-Fi on Orange Pi Zero
From: Alexey Kardashevskiy @ 2016-11-30 2:11 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Mark Rutland, devicetree, Vishnu Patekar, Arnd Bergmann,
linux-doc, Andre Przywara, Jonathan Corbet, Russell King, LKML,
Hans de Goede, Chen-Yu Tsai, Maxime Ripard, linux-arm-kernel
In-Reply-To: <20161129131922.JFbeipav@smtp2o.mail.yandex.net>
On 29/11/16 21:19, Icenowy Zheng wrote:
>
> 2016年11月29日 15:16于 Alexey Kardashevskiy <aik@ozlabs.ru>写道：
>>
>>
>>
>> On Wed, Nov 23, 2016 at 6:59 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
>>>
>>> Hi,
>>>
>>> On Tue, Nov 22, 2016 at 12:24:21AM +0800, Icenowy Zheng wrote:
>>> > There's a Allwinner's XR819 SDIO Wi-Fi module soldered on the board of
>>> > Orange Pi Zero, which used a dedicated regulator to power.
>>> >
>>> > Add the device tree node of the regulator, the enable gpio (with
>>> > mmc-pwrseq) and the sdio controller.
>>> >
>>> > There's a out-of-tree driver tested to work with this device tree.
>>
>>
>> btw could you please give a pointer where to find a XR819 driver for
> relatively recent kernel (4.8 may be, just not 3.4)? Thanks.
>
> https://github.com/Icenowy/xradio
Thanks!
I tried, cannot make it work though.
This compiles:
CONFIG_XRADIO=y
CONFIG_XRADIO_SDIO=y
CONFIG_XRADIO_NON_POWER_OF_TWO_BLOCKSIZES=y
# CONFIG_XRADIO_USE_GPIO_IRQ is not set
# CONFIG_XRADIO_5GHZ_SUPPORT is not set
# CONFIG_XRADIO_WAPI_SUPPORT is not set
CONFIG_XRADIO_USE_EXTENSIONS=y
But produces:
[ 0.964793] [XRADIO] Driver Label:L34M.01.08.0002
[ 0.969659] [XRADIO] Allocated hw_priv @ df684dc0
[ 3.038167] [SBUS_ERR] sdio probe timeout!
[ 3.046283] [XRADIO_ERR] sbus_sdio_init failed
If I enable CONFIG_XRADIO_USE_GPIO_IRQ (which Kconfig suggest to enable -
do I really need it?), it does not compile:
drivers/built-in.o: In function `sdio_irq_subscribe':
sunxi_sid.c:(.text+0x12dfe8): undefined reference to `xradio_request_gpio_irq'
sunxi_sid.c:(.text+0x12e06c): undefined reference to `xradio_free_gpio_irq'
sunxi_sid.c:(.text+0x12e080): undefined reference to `sunxi_mci_check_r1_ready'
drivers/built-in.o: In function `sdio_irq_unsubscribe':
sunxi_sid.c:(.text+0x12e3b0): undefined reference to `xradio_free_gpio_irq'
Makefile:962: recipe for target 'vmlinux' failed
make: *** [vmlinux] Error 1
I am using recent upstream kernel and
https://patchwork.kernel.org/patch/9439759/
https://patchwork.kernel.org/patch/9439761/
and I added missing emac.
Basically, here is my current tree with all sun8xi support
https://github.com/aik/linux/commits/pizero
and I just did
git -C drivers/net/ethernet clone https://github.com/Icenowy/xradio
and compiled it into vmlinux (no modules).
What am I missing? Sorry I am a newbie with ARM. Thanks.
--
Alexey
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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