Re: [PATCH] Bluetooth: btusb: fix excessive stack usage
On Thu, Feb 4, 2021 at 7:47 AM Arnd Bergmann wrote:
>
> Enlarging the size of 'struct btmtk_hci_wmt_cmd' makes it no longer
>
> Unfortunately, I could not figure out why the message size is
> increased in the previous patch. Using dynamic allocation means
That patch appears to be have been split out of fc342c4dc40
"Bluetooth: btusb: Add protocol support for MediaTek MT7921U USB
devices". But there is no clear reason why those changes were split
out, which is not helped by vague patch description, and size increase
appears to be a totally random change to unrelated code. This struct
is used by that latter commit to download firmware with a new format
for mt7921.
But new firmware download function uses code that is just copied from
existing fw download function (should be refactored to share code),
which has a max packet data size of "dlen = min_t(int, 250,
dl_size);", so there was no need to increase size at all. I'd guess
someone experimented with larger chunks for firmware download, but
then did not use them, but left the larger max size in because it was
a separate commit.
It looks like the new firmware download function will crash if the
firmware file is not consistent:
sectionmap = (struct btmtk_section_map *)(fw_ptr +
MTK_FW_ROM_PATCH_HEADER_SIZE +
MTK_FW_ROM_PATCH_GD_SIZE + MTK_FW_ROM_PATCH_SEC_MAP_SIZE * i);
section_offset = sectionmap->secoffset;
dl_size = sectionmap->bin_info_spec.dlsize;
...
fw_ptr += section_offset;
/* send fw_ptr[0] to fw_ptr[dl_size] via wmt_cmd(s) */
Both section_offset and dl_size are used unsanitized from the firmware
blob and could point outside the blob.
And the manually calculated struct sizes aren't necessary, if the
structs for the firmware were correct, it could just be:
struct btmtk_firmware {
struct btmtk_patch_header header;
struct btmtk_global_desc desc;
struct btmtk_section_map sections[];
} __packed;
struct btmtk_firmware* fw_ptr = fw->data;
sectionmap = _ptr->sections[i];
[PATCH] Bluetooth: btusb: Always fallback to alt 1 for WBS
When alt mode 6 is not available, fallback to the kernel <= 5.7 behavior
of always using alt mode 1.
Prior to kernel 5.8, btusb would always use alt mode 1 for WBS (Wide
Band Speech aka mSBC aka transparent SCO). In commit baac6276c0a9
("Bluetooth: btusb: handle mSBC audio over USB Endpoints") this
was changed to use alt mode 6, which is the recommended mode in the
Bluetooth spec (Specifications of the Bluetooth System, v5.0, Vol 4.B
§2.2.1). However, many if not most BT USB adapters do not support alt
mode 6. In fact, I have been unable to find any which do.
In kernel 5.8, this was changed to use alt mode 6, and if not available,
use alt mode 0. But mode 0 has a zero byte max packet length and can
not possibly work. It is just there as a zero-bandwidth dummy mode to
work around a USB flaw that would prevent device enumeration if
insufficient bandwidth were available for the lowest isoc mode
supported.
In effect, WBS was broken for all USB-BT adapters that do not support
alt 6, which appears to nearly all of them.
Then in commit 461f95f04f19 ("Bluetooth: btusb: USB alternate setting 1 for
WBS") the 5.7 behavior was restored, but only for Realtek adapters.
I've tested a Broadcom BRCM20702A and CSR 8510 adapter, both work with
the 5.7 behavior and do not with the 5.8.
So get rid of the Realtek specific flag and use the 5.7 behavior for all
adapters as a fallback when alt 6 is not available. This was the
kernel's behavior prior to 5.8 and I can find no adapters for which it
is not correct. And even if there is an adapter for which this does not
work, the current behavior would be to fall back to alt 0, which can not
possibly work either, and so is no better.
Signed-off-by: Trent Piepho
---
drivers/bluetooth/btusb.c | 20 ++--
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 03b83aa91277..1b690164ab5b 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -506,7 +506,6 @@ static const struct dmi_system_id
btusb_needs_reset_resume_table[] = {
#define BTUSB_HW_RESET_ACTIVE 12
#define BTUSB_TX_WAIT_VND_EVT 13
#define BTUSB_WAKEUP_DISABLE 14
-#define BTUSB_USE_ALT1_FOR_WBS 15
struct btusb_data {
struct hci_dev *hdev;
@@ -1736,15 +1735,12 @@ static void btusb_work(struct work_struct *work)
new_alts = data->sco_num;
}
} else if (data->air_mode == HCI_NOTIFY_ENABLE_SCO_TRANSP) {
- /* Check if Alt 6 is supported for Transparent audio */
- if (btusb_find_altsetting(data, 6)) {
- data->usb_alt6_packet_flow = true;
- new_alts = 6;
- } else if (test_bit(BTUSB_USE_ALT1_FOR_WBS,
>flags)) {
- new_alts = 1;
- } else {
- bt_dev_err(hdev, "Device does not support ALT
setting 6");
- }
+ /* Bluetooth USB spec recommends alt 6 (63 bytes), but
+* many adapters do not support it. Alt 1 appears to
+* work for all adapters that do not have alt 6, and
+* which work with WBS at all.
+*/
+ new_alts = btusb_find_altsetting(data, 6) ? 6 : 1;
}
if (btusb_switch_alt_setting(hdev, new_alts) < 0)
@@ -4548,10 +4544,6 @@ static int btusb_probe(struct usb_interface *intf,
* (DEVICE_REMOTE_WAKEUP)
*/
set_bit(BTUSB_WAKEUP_DISABLE, >flags);
- if (btusb_find_altsetting(data, 1))
- set_bit(BTUSB_USE_ALT1_FOR_WBS, >flags);
- else
- bt_dev_err(hdev, "Device does not support ALT setting
1");
}
if (!reset)
--
2.26.2
Re: [PATCH v3 1/2] Bluetooth: btusb: define HCI packet sizes of USB Alts
On Tuesday, December 8, 2020 5:13:36 PM PST Pali Rohár wrote: > On Tuesday 08 December 2020 15:04:29 Trent Piepho wrote: > > Does this also give userspace a clear point at which to determine MTU setting, > > _before_ data is sent over SCO connection? It will not work if sco_mtu is not > > valid until after userspace sends data to SCO connection with incorrect mtu. > > IIRC connection is established after sync connection (SCO) complete > event. And sending data is possible after connection is established. So > based on these facts I think that userspace can determinate MTU settings > prior sending data over SCO socket. > > Anyway, to whole MTU issue for SCO there is a nice workaround which > worked fine with more tested USB adapters and headsets. As SCO socket is > synchronous and most bluetooth headsets have own clocks, you can > synchronize sending packets to headsets based on time events when you > received packets from other side and also send packets of same size as > you received. I.e. for every received packet send own packet of the same > size. As I understand it, the RX side from the headset is not guaranteed, so in the TX only case this will not work and we still need to be told what MTU kernel has selected for the SCO link. It seems also it would add some latency to start up, since it would be necessary to wait for packets to arrive before knowing what size packet to send. Would timing based on matching TX to RX in the case of packet loss on RX side?
Re: [PATCH v3 1/2] Bluetooth: btusb: define HCI packet sizes of USB Alts
On Wednesday, September 23, 2020 3:22:15 AM PST Pali Rohár wrote:
> On Monday 14 September 2020 20:18:27 Joseph Hwang wrote:
> > On Thu, Sep 10, 2020 at 4:18 PM Pali Rohár wrote:
> > > And this part of code which you write is Realtek specific.
> >
> > We currently only have Intel and Realtek platforms to test with. If
> > making it generic without proper testing platforms is fine, I will
> > make it generic. Or do you think it might be better to make it
> > customized with particular vendors for now; and make it generic later
> > when it works well with sufficient vendors?
>
> I understood that those packet size changes are generic to bluetooth
> specification and therefore it is not vendor specific code. Those packet
> sizes for me really seems to be USB specific.
>
> Therefore it should apply for all vendors, not only for Realtek and
> Intel.
I have tried to test WBS with some different USB adapters. So far, all use
these packet sizes. Tested were:
Broadcom BRCM20702A
Realtek RTL8167B
Realtek RTL8821C
CSR CSR8510 (probably fake)
In all cases, WBS works best with packet size of (USB packet size for alt mode
selected) * 3 packets - 3 bytes HCI header. None of these devices support alt
6 mode, where supposedly one packet is better, but I can find no BT adapter on
which to test this.
> +static const int hci_packet_size_usb_alt[] = { 0, 24, 48, 72, 96, 144, 60};
Note that the packet sizes here are based on the max isoc packet length for
the USB alt mode used, e.g. alt 1 is 9 bytes. That value is only a
"recommended" value from the bluetooth spec. It seems like it would be more
correct use (btusb_data*)->isoc_tx_ep->wMaxPacketSize to find the MTU.
> > [Issue 2] The btusb_work() is performed by a worker. There would be a
> > timing issue here if we let btusb_work() to do “hdev->sco_mtu =
> > hci_packet_size_usb_alt[i]” because there is no guarantee how soon the
> > btusb_work() can be finished and get “hdev->sco_mtu” value set
> > correctly. In order to avoid the potential race condition, I suggest
> > to determine air_mode in btusb_notify() before
> > schedule_work(>work) is executed so that “hdev->sco_mtu =
> > hci_packet_size_usb_alt[i]” is guaranteed to be performed when
> > btusb_notify() finished. In this way, hci_sync_conn_complete_evt() can
> > set conn->mtu correctly as described in [Issue 1] above.
Does this also give userspace a clear point at which to determine MTU setting,
_before_ data is sent over SCO connection? It will not work if sco_mtu is not
valid until after userspace sends data to SCO connection with incorrect mtu.
Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
On Wed, Sep 30, 2020 at 2:47 AM Tony Lindgren wrote: > > * Trent Piepho [200930 09:34]: > > > > Where do these flags go? In pinctrl-single,pins? Like: > > > > pinctrl-single,pins = ; > > > > But PIN_INPUT_PULLUP is a generic flag? Which is translated into the > > proper value by?? > > Yes that's what I was thinking, something like this with generic flags: > > pinctrl-single,pins = ; It doesn't seem like these patches help achieve that, since they create device tree binaries with a property that has the same name and number of cells, but the cells have a different meaning than above, and must be handled differently by the driver. So you either drop compatibility or need to forever deal with supporting an interim format that is being created by these patches. The conf and max are already split in (all but one) of the device tree SOURCE files via the macro with multiple fields. That does seem like a good step if you wanted to transition into something like the above. But splitting it in the binary too doesn't help. Is there a way the dtb now being created can turn into the above via a driver change? Absolutely not. So what's the point of an interim binary format? All that matters is what the source looks like, and since that doesn't change, nothing is accomplished. I'll also point out that the current generic pinconf, done not via flags but with multiple properties for each configurable parameter, has a drive strength properties with strength in mA or µA as a parameter. How would you do that with generic bit flags? I don't think you can fit everything in pinconf-generic.h into one 32 bit cell. > > Or are you talking about replacing the existing pinctrl-0, > > pinctrl-names properties with a totally different system that looks > > more like gpio and interrupt handles? > > That would be even better :) Might be just too much to deal with.. > > In any case the parser code could already be generic if we had generic > flags based on #pinctrl-cells. But the pinctrl-single,pins isn't parsed. It just uses the values as they are. You'd have to write something to parse the cells and add more data to the dts that told the parser how to turn them into device specific values. It seems like that could eventually achieve basically what is happening already with the dts preprocessor converting symbolic constants into device specific values.
Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
On Wed, Sep 30, 2020 at 2:15 AM Tony Lindgren wrote:
>
> * Trent Piepho [200930 08:35]:
> > The closest thing would be the generic pin config type bindings, which
> > go in the pinctrl driver's nodes, and look like this:
> > _pinmux {
> > pinctrl_yoyo_reset: yoyogrp {
> > pins = "foo";
> > function = "gpio";
> > bias-pull-up;
> > };
> > };
>
> There's a bit of a dtb size and boot time issue for adding properties
> for each pin where that needs to be done for several hundred pins :)
pins is list, multiple pins can be specified at once. Otherwise the
property name would be "pin" and not "pins" There's also a groups
property to refer to multiple pins at once, e.g.
arch/mips/boot/dts/ingenic/ci20.dts-pins_mmc1: mmc1 {
arch/mips/boot/dts/ingenic/ci20.dts-function = "mmc1";
arch/mips/boot/dts/ingenic/ci20.dts:groups =
"mmc1-1bit-d", "mmc1-4bit-d";
arch/mips/boot/dts/ingenic/ci20.dts-bias-disable;
arch/mips/boot/dts/ingenic/ci20.dts-};
arch/mips/boot/dts/pic32/pic32mzda_sk.dts- user_leds_s0: user_leds_s0 {
arch/mips/boot/dts/pic32/pic32mzda_sk.dts: pins = "H0", "H1", "H2";
arch/mips/boot/dts/pic32/pic32mzda_sk.dts- output-low;
arch/mips/boot/dts/pic32/pic32mzda_sk.dts- microchip,digital;
arch/mips/boot/dts/pic32/pic32mzda_sk.dts- };
> > Is "some additional property for specifying generic conf flags"
> > different from the existing pinctrl-single,bias-pullup, etc.
> > properties? Because splitting the data cell into two parts doesn't
> > make any difference to those.
>
> So with an interrupt style binding with generic pinconf flags we can
> leave out the parsing of multiple properties for each pin. Sure the
> pin is only referenced by the controller like you pointed out but the
> pinconf flags could be generic.
Where do these flags go? In pinctrl-single,pins? Like:
pinctrl-single,pins = ;
But PIN_INPUT_PULLUP is a generic flag? Which is translated into the
proper value by??
Or are you talking about replacing the existing pinctrl-0,
pinctrl-names properties with a totally different system that looks
more like gpio and interrupt handles?
Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
On Tue, Sep 29, 2020 at 10:15 PM Tony Lindgren wrote:
> * Trent Piepho [200929 20:16]:
> > On Thu, Sep 24, 2020 at 12:04 AM Tony Lindgren wrote:
> > > Certainly different compatible strings can be used as needed.
> > > But pinctrl-single is not going to be am335x specific though :)
> > > We have quite a few SoCs using it:
> >
> > So what doesn't make sense to me, is to put something am335x specific
> > like two cells for conf and mux, into a generic driver like pinctrl
> > single.
>
> Treating conf and mux separately is not am335x specific. Linux treats
> them separately because the conf options typically can be described
> in a generic way while the mux is just signal routing.
There's already a generic pinconf feature for pinctrl-single. It
seems like this could be used with am335x now without any changes,
e.g. by adding "pinctrl-single,drive-strength" to define the bits that
control drive strength. It doesn't require added cells to use this.
Pin mux and configuration fields all have masks defined.
Example, add this:
#define PULL_MASK (PULL_UP|PULL_DISABLE)
pinctrl-single,bias-pullup = ;
pinctrl-single,bias-pulldown = ;
If I read the driver right (the bindings doc is far from clear), then
I think that configures the pin with pad pull up/down disabled and
allows the generic pinconf system to enable the pullup or pulldown.
Combining the definition of the fields with the value to initialize it
in the same property doesn't make that much sense to me.
> With interrupts the IRQ_TYPE flags are generic and separate from the
> hardware specific cells. If we wanted to, we could have something
> similar for pinctrl framework.
pinctrl-cells is really pretty different from gpio-cells and
interrupt-cells. The latter two are used in handles that allow an
external node to reference something in the node that defines the gpio
or interrupt cells. The generic flags are used by an *unrelated
driver* to tell an platform specific interrupt controller driver it
should configure an edge triggered interrupt. It makes it easier to
use the binding in the unrelated driver that needs an interrupt since
the flags are always the same. But mainly it works because the gpio
and interrupt framework in the kernel already support these concepts,
so the flags can get passed as is to existing kernel functions.
But pinctrl-single,pins is only used inside pinctrl-single itself.
There's no handle anywhere like:
yoyodyne,reset = <_pinmux AM335X_PIN_FOO MUX_MODE7
GENERIC_PULLUP_ENABLED_FLAG>;
I don't see how one could get made either, since there's already a
pinctrl system and it just doesn't work that way.
The closest thing would be the generic pin config type bindings, which
go in the pinctrl driver's nodes, and look like this:
_pinmux {
pinctrl_yoyo_reset: yoyogrp {
pins = "foo";
function = "gpio";
bias-pull-up;
};
};
That would work on some other boards. To use pinctrl-single, you'd
need to have a binding that mapped pin and function names to numbers
(why couldn't the pincfg binding use numbers!) and the bits/mask for
pull up. Which could be done. But at that point pinctrl-single,pins
is replaced, it wouldn't even get used, so adding more cells to it
hasn't done anything for you.
> > Consider also that any future changes to the pinctrl-single bindings
> > would need to be backward compatible with a device tree binary where
> > two cells get combined. So if the bindings being added here aren't
> > done, then adding them now creates an unnecessary additional version
> > to deal with for backward compatibility.
>
> I don't see issues with backward compabilty. If we specify that the
> controller uses #pinctrl-cells = <2>, and some additional property
> for specifying generic conf flags, we'd have a similar generic binding
> to the interrupt binding.
Is "some additional property for specifying generic conf flags"
different from the existing pinctrl-single,bias-pullup, etc.
properties? Because splitting the data cell into two parts doesn't
make any difference to those.
Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
On Thu, Sep 24, 2020 at 12:04 AM Tony Lindgren wrote: > > * Trent Piepho [200924 06:31]: > > > > > > > > The pinctrl-single driver? How will that work with boards that are > > > > not am335x and don't use conf and mux fields in the same manner as > > > > am335x? > > > > > > For those cases we still have #pinctrl-cells = <1>. > > > > If pincntrl-single is going to be am335x specific, then shouldn't it > > be a different compatible string? > > Certainly different compatible strings can be used as needed. > But pinctrl-single is not going to be am335x specific though :) > We have quite a few SoCs using it: So what doesn't make sense to me, is to put something am335x specific like two cells for conf and mux, into a generic driver like pinctrl single. This series adds two cells ORed into one. Ok, that's generic, other platforms could use it. But it also accomplishes nothing, so what's the point? You've hinted there is more to come, which will accomplish something, but what is it? That can be: Used by platforms other than am335x Can't already be done with the pinctrl single pinconf features Needs more than one data cell per pin > > Are the driver changes something that can be not be done with the > > pinconf-single properties? They all include a mask. > > Sure but in the long term we're better off with using #pinctrl-cells > along the lines what we have for example for #interrupt-cells and > #gpio-cells. So if you look at gpio-cells, virtually every driver uses 2, where one cell is the gpio index and the other is a common set of flags. It's the standard layout of a gpio handle that almost all bindings use. Only a handful of platform specific gpio drivers have another cell to indicate bank (probably better to have made each bank its own device node). Interrupt controllers have different numbers of cells, but they are all platform specific, and the cells have defined platform specific meanings. pci-host-cam-generic is a somewhat generic interrupt controller and it uses 1 cell, since it lacks device specific fields to put into additional cells. So I don't see an example of multiple cells which do not have a defined meaning that applies to all devices using the bindings. Consider also that any future changes to the pinctrl-single bindings would need to be backward compatible with a device tree binary where two cells get combined. So if the bindings being added here aren't done, then adding them now creates an unnecessary additional version to deal with for backward compatibility.
Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
On Wed, Sep 23, 2020 at 11:06 PM Tony Lindgren wrote: > > * Trent Piepho [200924 05:49]: > > On Wed, Sep 23, 2020 at 10:43 PM Tony Lindgren wrote: > > > > > > * Trent Piepho [200924 01:34]: > > > > On Tue, Sep 22, 2020 at 11:57 PM Tony Lindgren wrote: > > > > > > > > > > Also FYI, folks have also complained for a long time that the > > > > > pinctrl-single > > > > > binding mixes mux and conf values while they should be handled > > > > > separately. > > > > > > > > > > > > > Instead of combining two fields when the dts is generated they are now > > > > combined when the pinctrl-single driver reads the dts. Other than > > > > this detail, the result is the same. The board dts source is the > > > > same. The value programmed into the pinctrl register is the same. > > > > There is no mechanism currently that can alter that value in any way. > > > > > > > > What does combining them later allow that is not possible now? > > > > > > It now allows further driver changes to manage conf and mux separately :) > > > > The pinctrl-single driver? How will that work with boards that are > > not am335x and don't use conf and mux fields in the same manner as > > am335x? > > For those cases we still have #pinctrl-cells = <1>. If pincntrl-single is going to be am335x specific, then shouldn't it be a different compatible string? Are the driver changes something that can be not be done with the pinconf-single properties? They all include a mask.
Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
On Wed, Sep 23, 2020 at 10:43 PM Tony Lindgren wrote: > > * Trent Piepho [200924 01:34]: > > On Tue, Sep 22, 2020 at 11:57 PM Tony Lindgren wrote: > > > > > > Also FYI, folks have also complained for a long time that the > > > pinctrl-single > > > binding mixes mux and conf values while they should be handled separately. > > > > > > > Instead of combining two fields when the dts is generated they are now > > combined when the pinctrl-single driver reads the dts. Other than > > this detail, the result is the same. The board dts source is the > > same. The value programmed into the pinctrl register is the same. > > There is no mechanism currently that can alter that value in any way. > > > > What does combining them later allow that is not possible now? > > It now allows further driver changes to manage conf and mux separately :) The pinctrl-single driver? How will that work with boards that are not am335x and don't use conf and mux fields in the same manner as am335x?
Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
On Tue, Sep 22, 2020 at 11:57 PM Tony Lindgren wrote: > > Also FYI, folks have also complained for a long time that the pinctrl-single > binding mixes mux and conf values while they should be handled separately. > Instead of combining two fields when the dts is generated they are now combined when the pinctrl-single driver reads the dts. Other than this detail, the result is the same. The board dts source is the same. The value programmed into the pinctrl register is the same. There is no mechanism currently that can alter that value in any way. What does combining them later allow that is not possible now?
Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
On Thu, Sep 17, 2020 at 2:20 AM Drew Fustini wrote: > > On Thu, Sep 17, 2020 at 02:03:46AM -0700, Trent Piepho wrote: > > On Mon, Sep 14, 2020 at 3:44 AM Drew Fustini wrote: > > > > > > + > > > +When #pinctrl-cells = 2, then setting a pin for a device could be done > > > with: > > > + > > > + pinctrl-single,pins = <0xdc 0x30 0x07>; > > > + > > > +Where 0x30 is the pin configuration value and 0x07 is the pin mux mode > > > value. > > > +See the device example and static board pins example below for more > > > information. > > > > Pin configuration and mux mode don't mean anything in pinctrl-single. > > On another machine, mux mode might not be programmed this way or even > > exist. Or the location of bits would probably be different, and this > > would seem to imply the 0x07 would get shifted to the correct location > > for where the pin mux setting was on that machine's pinctrl registers. > > > > It seems like it would be better to explain the values are ORed together. > > I descirbed it as seoerate values as I did not want to prescribe what > the pcs driver would do with those values. But, yes, it is a just an OR > operation, so I could change the language to reflect tat. If you don't say what the pinctrl-single driver does with the values, how would anyone know how to use it? > > What is the purpose of this change anyway? It seems like in the end > > it just does what it did before. The data is now split into two cells > > in the device tree, but why? > > These changes were a result of desire to seperate pinconf and pinmux. > Tony raised the idea in a thread at the end of May [1]. > > Tony wrote: > > Only slightly related, but we should really eventually move omaps to use > > #pinctrl-cells = <2> (or 3) instead of 1, and pass the pinconf seprately > > from the mux mode. We already treat them separately with the new > > AM33XX_PADCONF macro, so we'd only have to change one SoC at a time to > > use updated #pinctrl-cells. But I think pinctrl-single might need some > > changes before we can do that. I still don't see what the goal is here. Support generic pinconf? Also note that while AM33XX_PADCONF() is changed, there is an in tree board that doesn't use it, so it's broken now. I found this change when it broke my out of tree board, due to the dtsi change not being reflected in my board's pinctrl values.
Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
On Mon, Sep 14, 2020 at 3:44 AM Drew Fustini wrote: > > + > +When #pinctrl-cells = 2, then setting a pin for a device could be done with: > + > + pinctrl-single,pins = <0xdc 0x30 0x07>; > + > +Where 0x30 is the pin configuration value and 0x07 is the pin mux mode value. > +See the device example and static board pins example below for more > information. Pin configuration and mux mode don't mean anything in pinctrl-single. On another machine, mux mode might not be programmed this way or even exist. Or the location of bits would probably be different, and this would seem to imply the 0x07 would get shifted to the correct location for where the pin mux setting was on that machine's pinctrl registers. It seems like it would be better to explain the values are ORed together. What is the purpose of this change anyway? It seems like in the end it just does what it did before. The data is now split into two cells in the device tree, but why?
Re: [PATCH v4 2/2] ARM: dts: am33xx-l4: change #pinctrl-cells from 1 to 2
On Tuesday, June 30, 2020 6:33:20 PM PDT Drew Fustini wrote:
> Increase #pinctrl-cells to 2 so that mux and conf be kept separate. This
> requires the AM33XX_PADCONF macro in omap.h to also be modified to keep pin
> conf and pin mux values separate.
> --- a/arch/arm/boot/dts/am33xx-l4.dtsi
> +++ b/arch/arm/boot/dts/am33xx-l4.dtsi
> @@ -278,7 +278,7 @@ scm: scm@0 {
> am33xx_pinmux: pinmux@800 {
> compatible = "pinctrl-single";
> reg = <0x800 0x238>;
> - #pinctrl-cells = <1>;
> + #pinctrl-cells = <2>;
> #define AM33XX_IOPAD(pa, val)OMAP_IOPAD_OFFSET((pa), 0x0800)
> (val)
> -#define AM33XX_PADCONF(pa, dir, mux) OMAP_IOPAD_OFFSET((pa), 0x0800) ((dir)
> | (mux))
> +#define AM33XX_PADCONF(pa, conf, mux)OMAP_IOPAD_OFFSET((pa), 0x0800)
> (conf) (mux)
If a dts file uses am33xx_pinmux from am33xx-l4.dtsi, but does not use the
AM33XX_PADCONF() macro for all pin settings, like say it uses AM33XX_IOPAD(),
then the dtb will be totally broken as pin addresses and values will all be
off.
Similarly, using AM33XX_PADCONF() with a different pinctrl defined elsewhere
would also break.
In the latest linux-next kernel, I found one case of the former problem, in
am335x-guardian.dts.
The barebox bootloader had all the am33xx boards broken when the dts change
was imported without adding the OR-two-values special case to the pinctrl
driver. Which I then tracked to here.
Re: [PATCH v4 1/2] pinctrl: single: parse #pinctrl-cells = 2
On Tuesday, June 30, 2020 6:33:19 PM PDT Drew Fustini wrote:
> If "pinctrl-single,pins" has 3 arguments (offset, conf, mux), then
> pcs_parse_one_pinctrl_entry() does an OR operation on conf and mux to
> get the value to store in the register.
> - vals[found].val = pinctrl_spec.args[1];
> +
> + switch (pinctrl_spec.args_count) {
> + case 2:
> + vals[found].val = pinctrl_spec.args[1];
> + break;
> + case 3:
> + vals[found].val = (pinctrl_spec.args[1] |
pinctrl_spec.args[2]);
> + break;
> + }
>
> dev_dbg(pcs->dev, "%pOFn index: 0x%x value: 0x%x\n",
> pinctrl_spec.np, offset,
pinctrl_spec.args[1]);
If #pinctrl-cells value is greater than 2, nothing will set vals[found].val to
anything other than zero (from when it's calloc'ed) and the pinctrl will
silently be programmed to zero.
The debug printout was not change to print vals[found].val, so it will
continue to print the value of the 2nd cell.
The result is that a #pinctrl-cells of 3 will produce no warning or error,
program the pinctrl to zero, whilst at the same time emit debug log messages
that it is programming the expected values.
The device tree documentation still states that #pinctrl-cells must be 1 when
using pinctrl-single,pins. This new special case of ORing two values is not
documented.
Re: [PATCH v4 2/2] net: phy: dp83867: Add SGMII mode type switching
On Mon, 2019-09-09 at 20:19 +0300, Vitaly Gaiduk wrote: > This patch adds ability to switch beetween two PHY SGMII modes. > Some hardware, for example, FPGA IP designs may use 6-wire mode > which enables differential SGMII clock to MAC. > > + > + val = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL); > + /* SGMII type is set to 4-wire mode by default. > + * If we place appropriate property in dts (see above) > + * switch on 6-wire mode. > + */ > + if (dp83867->sgmii_ref_clk_en) > + val |= DP83867_SGMII_TYPE; > + else > + val &= ~DP83867_SGMII_TYPE; > + phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL, val); Should use phy_modify_mmd().
Re: [PATCH 1/2] net: phy: dp83867: Add documentation for SGMII mode type
On Thu, 2019-09-05 at 19:26 +0300, Vitaly Gaiduk wrote: > Add documentation of ti,sgmii-type which can be used to select > SGMII mode type (4 or 6-wire). > > Signed-off-by: Vitaly Gaiduk > --- > Documentation/devicetree/bindings/net/ti,dp83867.txt | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.txt > b/Documentation/devicetree/bindings/net/ti,dp83867.txt > index db6aa3f2215b..18e7fd52897f 100644 > --- a/Documentation/devicetree/bindings/net/ti,dp83867.txt > +++ b/Documentation/devicetree/bindings/net/ti,dp83867.txt > @@ -37,6 +37,7 @@ Optional property: > for applicable values. The CLK_OUT pin can also > be disabled by this property. When omitted, the > PHY's default will be left as is. > + - ti,sgmii-type - This denotes the fact which SGMII mode is used (4 or > 6-wire). Really should explain what kind of value it is and what the values mean. I.e., should this be ti,sgimii-type = <4> to select 4 wire? Maybe a boolean, "sgmii-clock", to indicate the presence of sgmii rx clock lines, would make more sense? I also wonder if phy-mode = "sgmii-clk" or "sgmii-6wire", vs the existing phy-mode = "sgmii", might also be a better way to describe this instead of a new property.
Re: [1/3] rtc/fsl: support flextimer for lx2160a
On Fri, 2019-08-23 at 17:57 +0800, Biwen Li wrote:
> The patch supports flextimer for lx2160a
>
> Signed-off-by: Biwen Li
> ---
> drivers/rtc/rtc-fsl-ftm-alarm.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/rtc/rtc-fsl-ftm-alarm.c b/drivers/rtc/rtc-fsl-
> ftm-alarm.c
> index 4f7259c2d6a3..2b81525f6db8 100644
> --- a/drivers/rtc/rtc-fsl-ftm-alarm.c
> +++ b/drivers/rtc/rtc-fsl-ftm-alarm.c
> @@ -313,6 +313,7 @@ static const struct of_device_id ftm_rtc_match[]
> = {
> { .compatible = "fsl,ls1088a-ftm-alarm", },
> { .compatible = "fsl,ls208xa-ftm-alarm", },
> { .compatible = "fsl,ls1028a-ftm-alarm", },
> + { .compatible = "fsl,lx2160a-ftm-alarm", },
> { },
> };
>
Since there's no data associated with each compatible, it doesn't seem
like there's any need to add a new one.
What's normally done is add two compatibles in the dts, the base
version and the specific version, e.g.:
+ rcpm: rcpm@1e34040 {
+ compatible = "fsl,lx2160a-rcpm", "fsl,qoriq-cpm-2.1+";
Or in this case, compatible = "fsl,lx2160a-ftm-alarm", "fsl,ls1088a-ftm-alarm";
Then there's no need to add to the driver list.
Re: [PATCH] rtc: snvs: fix possible race condition
On Fri, 2019-07-19 at 02:57 +, Anson Huang wrote: > > > I do worry that handling the irq before the rtc device is registered could > > still > > result in a crash. From what I saw, the irq path in snvs only uses driver > > state > > members that are fully initialized for the most part, and the allocated but > > unregistered data->rtc is only used in one call to rtc_update_irq(), which > > appears to be ok with this. > > > > But it is not that hard to imagine that something could go into the rtc core > > that assumes call like rtc_update_irq() are only made on registered devices. > > > > If there was a way to do it, I think allocating the irq in a masked state > > and > > then unmasking it as part of the final registration call to make the device > > go > > live would be a safer and more general pattern. > > It makes sense, I think we can just move the devm_request_irq() to after > rtc_register_device(), > It will make sure everything is ready before IRQ is enabled. Will send out a > V2 patch. That will mean registering the rtc, then unregistering it if the irq request fails. More of a pain to write this failure path. Alexandre, is it part of rtc core design that rtc_update_irq() might be called on a rtc device that is properly allocated, but not registered yet?
Re: [PATCH] rtc: snvs: fix possible race condition
On Thu, 2019-07-18 at 03:08 +, Aisheng Dong wrote: > > From: Anson Huang > > Sent: Wednesday, July 17, 2019 9:58 PM> > > Hi, Aisheng > > > > > > From: anson.hu...@nxp.com > > > > Sent: Tuesday, July 16, 2019 3:19 PM > > > > > > > > The RTC IRQ is requested before the struct rtc_device is > > > > allocated, > > > > this may lead to a NULL pointer dereference in IRQ handler. > > > > > > > > To fix this issue, allocating the rtc_device struct before > > > > requesting the RTC IRQ using devm_rtc_allocate_device, and use > > > > rtc_register_device to register the RTC device. > > > > > > > > > > I saw other rtc drivers did the same way as us, so this looks > > > like a > > > common problem. > > > My question is if we can clear interrupt status before register > > > to > > > avoid this issue as other rtc drivers? > > > > I think we can NOT predict when the IRQ will be pending, IRQ could > > arrive at > > any time, the most safe way is to prepare everything before > > requesting/enabling IRQ. > > There is also patch to fix similar issue: I think one could attempt to disable all irq sources in the device via its register space, then enable the interrupt. But this seems more specific to each device than changing the pattern of device registration, so IMHO, it's not really better. I do worry that handling the irq before the rtc device is registered could still result in a crash. From what I saw, the irq path in snvs only uses driver state members that are fully initialized for the most part, and the allocated but unregistered data->rtc is only used in one call to rtc_update_irq(), which appears to be ok with this. But it is not that hard to imagine that something could go into the rtc core that assumes call like rtc_update_irq() are only made on registered devices. If there was a way to do it, I think allocating the irq in a masked state and then unmasking it as part of the final registration call to make the device go live would be a safer and more general pattern. >
Re: [PATCH] rtc: Don't state that the RTC holds UTC in case it doesn't
On Tue, 2019-06-25 at 21:19 +0200, Alexandre Belloni wrote: > On 25/06/2019 17:16:52+0000, Trent Piepho wrote: > > On Tue, 2019-06-25 at 11:29 +0200, Alexandre Belloni wrote: > > > Userspace is certainly adjusting the timezone after the kernel did. Can > > > you run the same commands without running your init? > > > > > > On stable, you have /etc/init.d/hwclock.sh that still runs and does the > > > correct thing. My understanding is that systemd also handles the TZ > > > properly after hctosys (see clock_is_localtime()). > > > > > > Seriously, hctosys does a really bad job at setting the system time, it > > > is guaranteed to be always wrong on most platforms. My plan is still to > > > try to get distros to stop enabling it and do that properly in > > > userspace. This is already ok when using sysV but systemd would need a > > > few changes to stop relying on it when then is no hwclock initscript. > > > Unfortunately, I didn't have time to work on that yet. > > > > hctosys is very handy in that it sets the system time before any log > > messages are generated. Either in a main boot or in an initramfs. > > Having property time-stamped log messages is very important for > > managing a large deployment. > > > > If the system time is set by some script or systemd unit, then there > > will always be all the things that need to run before that script or > > unit can work. E.g., udev creating rtc device nodes, mounting /sys and > > /proc, systemd generator for local file system unis, the other parts of > > systemd to do that, etc. All this won't be able to log with correct > > system time. > > > > I'd argue that this system time is not correct anyway and hence is not > that useful. Especially since the boot time to anything reading the RTC > will still be smaller than the maximum error hctosys can have (that is > up to two seconds). This is even worse if the RTC sotres local time. Why would the system time be incorrect by more than +- 500 ms plus whatever the drift the RTC since the last time it was correctly set? Is there another source of error? The +-500 ms error could be reduced by using an update interrupt in hctosys. If one were to not set the RTC to UTC, then yes there is a problem. But it's manageable. Don't do that. I've got large numbers of embedded devices I've designed. Why would I design them to use local time in the RTC? If one does set the RTC to local time, then it's bad, but not impossible to deal with. The log messages that have timestamps between boot and when the timezone is set will be in the wrong zone. The necessary information to correct this error exists. It's far better than all timestamps as Jan 1st 1970. > Instead of using a systemd unit, we could make systemd read the RTC as > soon as possible and set the system time correctly (at least, that is my But what is as soon as possible? Doesn't it need /dev/rtc or maybe /sys/class/rtc to exist? That doesn't happen for a while after boot. > plan). This would be especially useful when using NTP because as already > reported, it may take a few hours to actually synchronize because > hctosys didn't set the correct time. Is that really a problem in hctosys? Usually one always does some sort of makestep option in ntp to set rather than skew the clock on start, perhaps only if the error is too great, to avoid synchronizing taking an exceeding long time. I see this as two blatant system misconfigurations. One, telling the kernel to set the time based on a RTC set to UTC and then not setting the RTC to UTC. And two, setting ntp to not step the clock and giving it a huge initial error to deal with. > > I would agree that there are remaining use cases for hctosys, e.g. when > wanting to boot a rootfs over a network filesystem and without an > initramfs. >
Re: [PATCH] rtc: Don't state that the RTC holds UTC in case it doesn't
On Tue, 2019-06-25 at 11:29 +0200, Alexandre Belloni wrote: > > > Userspace is certainly adjusting the timezone after the kernel did. Can > you run the same commands without running your init? > > On stable, you have /etc/init.d/hwclock.sh that still runs and does the > correct thing. My understanding is that systemd also handles the TZ > properly after hctosys (see clock_is_localtime()). > > Seriously, hctosys does a really bad job at setting the system time, it > is guaranteed to be always wrong on most platforms. My plan is still to > try to get distros to stop enabling it and do that properly in > userspace. This is already ok when using sysV but systemd would need a > few changes to stop relying on it when then is no hwclock initscript. > Unfortunately, I didn't have time to work on that yet. hctosys is very handy in that it sets the system time before any log messages are generated. Either in a main boot or in an initramfs. Having property time-stamped log messages is very important for managing a large deployment. If the system time is set by some script or systemd unit, then there will always be all the things that need to run before that script or unit can work. E.g., udev creating rtc device nodes, mounting /sys and /proc, systemd generator for local file system unis, the other parts of systemd to do that, etc. All this won't be able to log with correct system time.
Re: [PATCH 2/3] rtc: imx-sc: Make compatible string more generic
On Tue, 2019-06-11 at 17:32 -0300, Fabio Estevam wrote:
> Hi Anson,
>
> On Tue, Jun 11, 2019 at 3:31 AM wrote:
> >
> > From: Anson Huang
> >
> > i.MX system controller RTC driver can support all i.MX SoCs
> > with system controller inside, this patch makes the compatible
> > string more generic to support other i.MX SoCs with system
> > controller inside, such as i.MX8QM etc..
> >
> > Signed-off-by: Anson Huang
> > ---
> > drivers/rtc/rtc-imx-sc.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/rtc/rtc-imx-sc.c b/drivers/rtc/rtc-imx-sc.c
> > index c933045..38ef3ca 100644
> > --- a/drivers/rtc/rtc-imx-sc.c
> > +++ b/drivers/rtc/rtc-imx-sc.c
> > @@ -178,7 +178,7 @@ static int imx_sc_rtc_probe(struct
> > platform_device *pdev)
> > }
> >
> > static const struct of_device_id imx_sc_dt_ids[] = {
> > - { .compatible = "fsl,imx8qxp-sc-rtc", },
> > + { .compatible = "fsl,imx-sc-rtc", },
>
> What is wrong with the current compatible string?
>
> If you want to support i.MX8QM just add in its dtsi:
>
> compatible = "fsl,imx8qm-sc-rtc", "fsl,imx8qxp-sc-rtc"
>
> and add a dt-bindings entry for "fsl,imx8qm-sc-rtc"
Yes, I thought this is (was?) the recommended practice for IP blocks in
SoCs that don't have their own version (vs something like a Synopsis
block used many places):
* Use the first SoC to have the block as the base compatible for the
block.
* Always add the current SoC, in additional to the base, in the SoC's
dts's list of compatibles. Even if unneeded at the present.
* The driver will list the base compatible in the match table, and will
add new ones for specific socs if/when there is a need for it.
Re: [PATCH 5/5] net: phy: dp83867: Use unsigned variables to store unsigned properties
On Mon, 2019-05-13 at 22:46 +0200, Andrew Lunn wrote:
> > > Perhaps you could tell me if the approach I've taken in patch 3,
> > > "Add ability to disable output clock", and patch 4, "Disable tx/rx
> > > delay when not configured", are considered acceptable? I can conceive
> > > of arguments for alternate approaches. I would like to add support for
> > > these into u-boot too, but typically u-boot follows the kernel DT
> > > bindings, so I want to finalize the kernel DT semantics before sending
> > > patches to u-boot.
> > >
> >
> > I lack experience with these TI PHY's. Maybe Andrew or Florian can advise.
>
> Hi Trent
>
> I already deleted the patches. For patch 3:
>
> + if (dp83867->clk_output_sel > DP83867_CLK_O_SEL_REF_CLK &&
> + dp83867->clk_output_sel != DP83867_CLK_O_SEL_OFF) {
> + phydev_err(phydev, "ti,clk-output-sel value %u out of
> range\n",
> +dp83867->clk_output_sel);
> + return -EINVAL;
> + }
>
> This last bit looks odd. If it is not OFF, it is invalid?
The valid values are in the range 0 to DP83867_CLK_O_SEL_REF_CLK and
also DP83867_CLK_O_SEL_OFF. Thus invalid values are those greater than
DP83867_CLK_O_SEL_REF_CLK which are not DP83867_CLK_O_SEL_OFF.
>
> Are there any in tree users of DP83867_CLK_O_SEL_REF_CLK? We have to
> be careful changing its meaning. But if nobody is actually using it...
Nope. I doubt this will affect anyone. They'd need to strap the phy
to get a different configuration, and the explicitly add a property,
which isn't in the example DTS files, to change the configuration to
something they didn't want, and then depend on a driver bug ignoring
the erroneous setting they added.
> Patch 4:
>
> This is harder. Ideally we want to fix this. At some point, somebody
> is going to want 'rgmii' to actually mean 'rgmii', because that is
> what their hardware needs.
>
> Could you add a WARN_ON() for 'rgmii' but the PHY is actually adding a
> delay? And add a comment about setting the correct thing in device
> tree? Hopefully we will then get patches correcting DT blobs. And if
> we later do need to fix 'rgmii', we will break less board.
Yes I can do this. Should it warn on any use of "rgmii"? If so, how
would someone make the warning go away if they actually want rgmii mode
with no delay?
I suspect hsdk.dts is an example of an in-tree broken board that uses
"rgmii" would it should have used "rgmii-id". Unfortunately, phy dts
nodes don't usually provide a compatible that identifies the phy, using
instead run-time auto-detection based on PHY id registers, so there's
no way to tell from the dts files what boards use this phy unless they
also specify one of the phy specific properties. Which is how I found
hsdk.dts (and no other boards).
[PATCH 5/5] net: phy: dp83867: Use unsigned variables to store unsigned properties
The variables used to store u32 DT properties were signed ints. This
doesn't work properly if the value of the property were to overflow.
Use unsigned variables so this doesn't happen.
Cc: Andrew Lunn
Cc: Florian Fainelli
Cc: Heiner Kallweit
Signed-off-by: Trent Piepho
---
drivers/net/phy/dp83867.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index a46cc9427fb3..edd9e27425e8 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -82,9 +82,9 @@ enum {
};
struct dp83867_private {
- int rx_id_delay;
- int tx_id_delay;
- int fifo_depth;
+ u32 rx_id_delay;
+ u32 tx_id_delay;
+ u32 fifo_depth;
int io_impedance;
int port_mirroring;
bool rxctrl_strap_quirk;
--
2.14.5
[PATCH 3/5] net: phy: dp83867: Add ability to disable output clock
Generally, the output clock pin is only used for testing and only serves
as a source of RF noise after this. It could be used to daisy-chain
PHYs, but this is uncommon. Since the PHY can disable the output, make
doing so an option. I do this by adding another enumeration to the
allowed values of ti,clk-output-sel.
The code was not using the value DP83867_CLK_O_SEL_REF_CLK as one might
expect: to select the REF_CLK as the output. Rather it meant "keep
clock output setting as is", which, depending on PHY strapping, might
not be outputting REF_CLK.
Change this so DP83867_CLK_O_SEL_REF_CLK means enable REF_CLK output.
Omitting the property will leave the setting as is (which was the
previous behavior in this case).
Out of range values were silently converted into
DP83867_CLK_O_SEL_REF_CLK. Change this so they generate an error.
Cc: Andrew Lunn
Cc: Florian Fainelli
Cc: Heiner Kallweit
Signed-off-by: Trent Piepho
---
drivers/net/phy/dp83867.c | 36 ++--
1 file changed, 26 insertions(+), 10 deletions(-)
diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index fd35131a0c39..420729cd6025 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -68,6 +68,7 @@
#define DP83867_IO_MUX_CFG_IO_IMPEDANCE_MAX0x0
#define DP83867_IO_MUX_CFG_IO_IMPEDANCE_MIN0x1f
+#define DP83867_IO_MUX_CFG_CLK_O_DISABLE BIT(6)
#define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK (0x1f << 8)
#define DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT 8
@@ -87,7 +88,8 @@ struct dp83867_private {
int io_impedance;
int port_mirroring;
bool rxctrl_strap_quirk;
- int clk_output_sel;
+ bool set_clk_output;
+ u32 clk_output_sel;
};
static int dp83867_ack_interrupt(struct phy_device *phydev)
@@ -154,11 +156,16 @@ static int dp83867_of_init(struct phy_device *phydev)
/* Optional configuration */
ret = of_property_read_u32(of_node, "ti,clk-output-sel",
>clk_output_sel);
- if (ret || dp83867->clk_output_sel > DP83867_CLK_O_SEL_REF_CLK)
- /* Keep the default value if ti,clk-output-sel is not set
-* or too high
-*/
- dp83867->clk_output_sel = DP83867_CLK_O_SEL_REF_CLK;
+ /* If not set, keep default */
+ if (!ret) {
+ dp83867->set_clk_output = true;
+ if (dp83867->clk_output_sel > DP83867_CLK_O_SEL_REF_CLK &&
+ dp83867->clk_output_sel != DP83867_CLK_O_SEL_OFF) {
+ phydev_err(phydev, "ti,clk-output-sel value %u out of
range\n",
+ dp83867->clk_output_sel);
+ return -EINVAL;
+ }
+ }
if (of_property_read_bool(of_node, "ti,max-output-impedance"))
dp83867->io_impedance = DP83867_IO_MUX_CFG_IO_IMPEDANCE_MAX;
@@ -288,11 +295,20 @@ static int dp83867_config_init(struct phy_device *phydev)
dp83867_config_port_mirroring(phydev);
/* Clock output selection if muxing property is set */
- if (dp83867->clk_output_sel != DP83867_CLK_O_SEL_REF_CLK)
+ if (dp83867->set_clk_output) {
+ u16 mask = DP83867_IO_MUX_CFG_CLK_O_DISABLE;
+
+ if (dp83867->clk_output_sel == DP83867_CLK_O_SEL_OFF) {
+ val = DP83867_IO_MUX_CFG_CLK_O_DISABLE;
+ } else {
+ mask |= DP83867_IO_MUX_CFG_CLK_O_SEL_MASK;
+ val = dp83867->clk_output_sel <<
+ DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT;
+ }
+
phy_modify_mmd(phydev, DP83867_DEVADDR, DP83867_IO_MUX_CFG,
- DP83867_IO_MUX_CFG_CLK_O_SEL_MASK,
- dp83867->clk_output_sel <<
- DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT);
+ mask, val);
+ }
return 0;
}
--
2.14.5
[PATCH 4/5] net: phy: dp83867: Disable tx/rx delay when not configured
The code was assuming the reset default of the delay control register
was to have delay disabled. This is what the datasheet shows as the
register's initial value. However, that's not actually true: the
default is controlled by the PHY's pin strapping.
If the interface mode is selected as RX or TX delay only, insure the
other direction's delay is disabled.
If the interface mode is just "rgmii", with neither TX or RX internal
delay, one might expect that the driver should disable both delays. But
this is not what the driver does. It leaves the setting at the PHY's
strapping's default. And that default, for no pins with strapping
resistors, is to have delay enabled and 2.00 ns.
Rather than change this behavior, I've kept it the same and documented
it. No delay will most likely not work and will break ethernet on any
board using "rgmii" mode.
Cc: Andrew Lunn
Cc: Florian Fainelli
Cc: Heiner Kallweit
Signed-off-by: Trent Piepho
---
drivers/net/phy/dp83867.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 420729cd6025..a46cc9427fb3 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -256,10 +256,16 @@ static int dp83867_config_init(struct phy_device *phydev)
return ret;
}
+ /* If rgmii mode with no internal delay is selected, we do NOT use
+* aligned mode as one might expect. Instead we use the PHY's default
+* based on pin strapping. And the "mode 0" default is to *use*
+* internal delay with a value of 7 (2.00 ns).
+*/
if ((phydev->interface >= PHY_INTERFACE_MODE_RGMII_ID) &&
(phydev->interface <= PHY_INTERFACE_MODE_RGMII_RXID)) {
val = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_RGMIICTL);
+ val &= ~(DP83867_RGMII_TX_CLK_DELAY_EN |
DP83867_RGMII_RX_CLK_DELAY_EN);
if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
val |= (DP83867_RGMII_TX_CLK_DELAY_EN |
DP83867_RGMII_RX_CLK_DELAY_EN);
--
2.14.5
Re: [PATCH] rtc: st-lpc: remove unnecessary check
On Wed, 2019-05-01 at 16:25 +0200, Alexandre Belloni wrote: > On 30/04/2019 22:31:19+0000, Trent Piepho wrote: > > On Tue, 2019-04-30 at 22:18 +0200, Alexandre Belloni wrote: > > > The RTC core already ensures the alarm is set to a time in the future, it > > > is not necessary to check again in the driver. > > > > My reading of the rtc core code is that it checks if the alarm is in > > the future *twice* before handing off the set call to the driver, which > > possibly checks a 3rd time (as seen here). > > > > However, all these checks are done *before* setting the alarm. It > > still possible to have a race and set the alarm after the time has > > already passed, in which case the alarm will never fire. > > > > I agree the core need to handle that possible race better and this is > something I'm planning to work on. > > > The way to fix the race would be to have the driver check the alarm > > *after* setting it. In precisely this order, do these steps: > > > > 1. Set alarm in RTC, to Talarm > > 2. Get time from RTC, as Tcurrent > > 3. Get alarm status from RTC > > > > If Talarm < Tcurrent, alarm was set to future time, no error > > This should be Talarm > Tcurrent, right? Yes. I wrote that backward. > > Else > > If status == fired, alarm was set and has since fired, no error > > Else status == not fired, alarm was set in past, EINVAL > > > > This should be race free. > > > > > > > > > > - /* Invalid alarm time */ > > > - if (now_secs > alarm_secs) > > > - return -EINVAL; > > > - > > > memcpy(>alarm, t, sizeof(struct rtc_wkalrm)); > > > > > > /* Now many secs to fire */ > > > > alarm_secs -= now_secs; > > lpa = (unsigned long long)alarm_secs * rtc->clkrate; > > > > While it's true the time wouldn't normally be in past, it still races, > > as describe above. In that case, the math here underflows alarm_secs, > > so it probably still makes sense to check. > > I can't believe you can possibly have more than one second between the > check in the core and the check in the driver, it doesn't make much > sense to check, even in the current state of the core. It's certainly possible to have multiple seconds pass. For an external device over SPI or I2C, one has to wait for the bus to become free. And on SPI that requires the kernel thread running the bus to be scheduled. Just put in some real-time tasks and maybe a big transfer to a flash chip and it could be a while before that happens. I don't think this device has that issue as I don't think it's external. And ever for a device on an external bus, delays > 1 second are unlikely. Possible, but unlikely. You can also get them when Linux is running under a hypervisor, i.e. a Linux VM. But also something like an NMI and ACPI BIOS. If the Linux guest is not scheduled to run for while anything that is supposed to be based on real time, like the value returned by an RTC, will still advance. It is possible that multiple seconds elapse from the guest CPU executing one instruction to the next. But even ignoring that, does it require > 1 second to elapse. Can't it happen when the clock ticks from one second to the next, which happens effectively instantly? If the time from the check to the time when the alarm is set is 1 microsecond, and the time this call to set the alarm is made is randomly done and not synchronized to the RTC, then isn't there a 1 out of 1 million chance (1 microsecond / 1 second), that the once per second clock tick will hit our 1 us window?
Re: [PATCH] rtc: st-lpc: remove unnecessary check
On Tue, 2019-04-30 at 22:18 +0200, Alexandre Belloni wrote: > The RTC core already ensures the alarm is set to a time in the future, it > is not necessary to check again in the driver. My reading of the rtc core code is that it checks if the alarm is in the future *twice* before handing off the set call to the driver, which possibly checks a 3rd time (as seen here). However, all these checks are done *before* setting the alarm. It still possible to have a race and set the alarm after the time has already passed, in which case the alarm will never fire. The way to fix the race would be to have the driver check the alarm *after* setting it. In precisely this order, do these steps: 1. Set alarm in RTC, to Talarm 2. Get time from RTC, as Tcurrent 3. Get alarm status from RTC If Talarm < Tcurrent, alarm was set to future time, no error Else If status == fired, alarm was set and has since fired, no error Else status == not fired, alarm was set in past, EINVAL This should be race free. > > - /* Invalid alarm time */ > - if (now_secs > alarm_secs) > - return -EINVAL; > - > memcpy(>alarm, t, sizeof(struct rtc_wkalrm)); > > /* Now many secs to fire */ alarm_secs -= now_secs; lpa = (unsigned long long)alarm_secs * rtc->clkrate; While it's true the time wouldn't normally be in past, it still races, as describe above. In that case, the math here underflows alarm_secs, so it probably still makes sense to check.
Re: [PATCH] rtc: snvs: Use __maybe_unused instead of #if CONFIG_PM_SLEEP
On Mon, 2019-04-29 at 07:02 +, Anson Huang wrote:
> Use __maybe_unused for power management related functions
> instead of #if CONFIG_PM_SLEEP to simply the code.
>
> Signed-off-by: Anson Huang
This will result in the functions always being included, even if
PM_SLEEP is off...
>
> @@ -387,14 +385,6 @@ static const struct dev_pm_ops snvs_rtc_pm_ops = {
> .resume_noirq = snvs_rtc_resume_noirq,
> };
...because they will always be used by the definition of
snvs_rtc_pm_ops here.
In order for this to work, SIMPLE_DEV_PM_OPS() needs to be used,
so that the dev_pm_ops struct is empty when PM is off and the functions
don't get referenced. See: https://lkml.org/lkml/2019/1/17/376
[PATCH] lib: Fix possible incorrect result from rational fractions helper
In some cases the previous algorithm would not return the closest
approximation. This would happen when a semi-convergent was the
closest, as the previous algorithm would only consider convergents.
As an example, consider an initial value of 5/4, and trying to find the
closest approximation with a maximum of 4 for numerator and denominator.
The previous algorithm would return 1/1 as the closest approximation,
while this version will return the correct answer of 4/3.
To do this, the main loop performs effectively the same operations as it
did before. It must now keep track of the last three approximations,
n2/d2 .. n0/d0, while before it only needed the last two.
If an exact answer is not found, the algorithm will now calculate the
best semi-convergent term, t, which is a single expression with two
divisions:
min((max_numerator - n0) / n1, (max_denominator - d0) / d1)
This will be used if it is better than previous convergent. The test
for this is generally a simple comparison, 2*t > a. But in an edge
case, where the convergent's final term is even and the best allowable
semi-convergent has a final term of exactly half the convergent's final
term, the more complex comparison (d0*dp > d1*d) is used.
I also wrote some comments explaining the code. While one still needs
to look up the math elsewhere, they should help a lot to follow how the
code relates to that math.
Cc: Oskar Schirmer
Signed-off-by: Trent Piepho
---
lib/rational.c | 63 +++---
1 file changed, 50 insertions(+), 13 deletions(-)
diff --git a/lib/rational.c b/lib/rational.c
index ba7443677c90..31fb27db2deb 100644
--- a/lib/rational.c
+++ b/lib/rational.c
@@ -3,6 +3,7 @@
* rational fractions
*
* Copyright (C) 2009 emlix GmbH, Oskar Schirmer
+ * Copyright (C) 2019 Trent Piepho
*
* helper functions when coping with rational numbers
*/
@@ -10,6 +11,7 @@
#include
#include
#include
+#include
/*
* calculate best rational approximation for a given fraction
@@ -33,30 +35,65 @@ void rational_best_approximation(
unsigned long max_numerator, unsigned long max_denominator,
unsigned long *best_numerator, unsigned long *best_denominator)
{
- unsigned long n, d, n0, d0, n1, d1;
+ /* n/d is the starting rational, which is continually
+* decreased each iteration using the Euclidean algorithm.
+*
+* dp is the value of d from the prior iteration.
+*
+* n2/d2, n1/d1, and n0/d0 are our successively more accurate
+* approximations of the rational. They are, respectively,
+* the current, previous, and two prior iterations of it.
+*
+* a is current term of the continued fraction.
+*/
+ unsigned long n, d, n0, d0, n1, d1, n2, d2;
n = given_numerator;
d = given_denominator;
n0 = d1 = 0;
n1 = d0 = 1;
+
for (;;) {
- unsigned long t, a;
- if ((n1 > max_numerator) || (d1 > max_denominator)) {
- n1 = n0;
- d1 = d0;
- break;
- }
+ unsigned long dp, a;
+
if (d == 0)
break;
- t = d;
+ /* Find next term in continued fraction, 'a', via
+* Euclidean algorithm.
+*/
+ dp = d;
a = n / d;
d = n % d;
- n = t;
- t = n0 + a * n1;
+ n = dp;
+
+ /* Calculate the current rational approximation (aka
+* convergent), n2/d2, using the term just found and
+* the two prior approximations.
+*/
+ n2 = n0 + a * n1;
+ d2 = d0 + a * d1;
+
+ /* If the current convergent exceeds the maxes, then
+* return either the previous convergent or the
+* largest semi-convergent, the final term of which is
+* found below as 't'.
+*/
+ if ((n2 > max_numerator) || (d2 > max_denominator)) {
+ unsigned long t = min((max_numerator - n0) / n1,
+ (max_denominator - d0) / d1);
+
+ /* This tests if the semi-convergent is closer
+* than the previous convergent.
+*/
+ if (2u * t > a || (2u * t == a && d0 * dp > d1 * d)) {
+ n1 = n0 + t * n1;
+ d1 = d0 + t * d1;
+ }
+ break;
+ }
n0 = n1;
- n1 = t;
- t = d0 + a * d1;
+ n1 = n2;
d0 = d1;
- d1 = t;
+ d1 = d2;
}
*best_numerator = n1
Re: Bug in spi: imx: Add support for SPI Slave mode
On Wed, 2019-02-27 at 10:55 +0900, Jiada Wang wrote:
> Hi Trent
>
> Thanks for reporting
Thanks for the information, it was very helpful!
>
> in the commit message of commit ("spi: imx: Add support for SPI Slave
> mode"), it mentions
>
> "The stale data in RXFIFO will be dropped when the Slave does any new
> transfer"
Sorry, somehow I totally missed that.
>
> > If it's necessary to empty the fifo before an xfer, then how did this
> > driver ever work before this change? I see no change anywhere else
> > that would make this a new requirement.
> >
>
> only slave mode needs to flush stale data in RXFIFO,
Ok, so maybe I can move this into a slave mode only path. That could
avoid the check in master mode, if it's not necessary there.
Ok course, that doesn't really fix my underlying "dma then pio"
problem, as even if it avoids corrupting memory, as there is still data
in the RX fifo where there should not be.
> Currently I am not able to test and this patch was created several years
> before, but as I can recall, the purpose is to flush stale data in RXFIFO
> before spi->rx_buf is set, but seems there is bug, after the first xfer,
> rx_buf will always point to somewhere, which can lead to memory corruption.
>
> > In my test, switching from DMA to PIO, which happens if the 1st xfer is
> > large enough to pass a >fifo/2 size test, and uses DMA, and the 2nd
> > xfer is smaller, and will use PIO, results in the PIO xfer trying to
> > empty the rx buffer in this code above. If there is not enough space
> > in the xfer's rx buffer, and there is no reason for there to be any
> > space at all, it clobbers memory.
> >
> > I suspect the author of this wasn't aware that spi_imx->rx() will write
> > the data received into the current xfer's rx buffer rather than throw
> > it away, and never tested this actually getting used for a transfer
> > where the rx data mattered.
> >
>
> yes, you are right,
> as I don't have access to HW, can you please submit a fix
> (for example reset rx_buf after each xfer?)
> > Still, I'd like to know why the flush rx thing is even here. Nothing
> > in the commit message or any discussion I could find addressed this.
> >
>
> master side may xfer data longer than slave side is expecting,
> the extra data received in RXFIFO, need to be dropped before new xfer
Yes, that makes a lot of sense why this would be needed.
I wonder if putting the flush after the xfer might be too early. If
the master is sending more data than expected, how do we knows it has
stopped when we do the post xfer flush? Perhaps the master sends more
after the flush is finished. Is there some point where additional data
into the RX fifo is disabled?
Unfortunately, I don't have hardware to test slave mode. Or even
master mode RX, as my hardware is TX only. The imx spi is just
receiving zeros to put into a spi core provided dummy buffer. I can
stop overflow the buffer, but I wouldn't know if the data that is in it
is correct.
Re: [PATCH] PCI: imx6: Don't request "pci_aux" clock on i.MX7
On Fri, 2019-03-01 at 00:55 -0800, Andrey Smirnov wrote:
> The clock in question is not present on i.MX7, so move the code
> requesting it into i.MX8MQ-only path.
>
> Fixes: eeb61c4e8530 ("PCI: imx6: Add code to request/control
> "pcie_aux" clock for i.MX8MQ")
> Reported-by: Trent Piepho
> Signed-off-by: Andrey Smirnov
> Cc: Bjorn Helgaas
> Cc: Chris Healy
> Cc: Lucas Stach
> Cc: Trent Piepho
> Cc: linux-...@nxp.com
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-...@vger.kernel.org
Tested on linux-next, imx7d PCI-e appears to be working fine now.
It's too bad git diff can't report the case label(s) code falls under
the same way it can (usually) get the function name. I saw the
original commit when it was posted and didn't notice anything myself
either.
Re: [PATCH 2/2] PCI: imx6: Add code to request/control "pcie_aux" clock for i.MX8MQ
On Tue, 2019-02-12 at 10:36 +0100, Lucas Stach wrote:
> Am Montag, den 11.02.2019, 17:51 -0800 schrieb Andrey Smirnov:
> > PCIe IP block has additional clock, "pcie_aux", that needs to be
> > controlled by the driver. Add code to support that.
This breaks iMX7d.
> >
> > @@ -1049,6 +1059,12 @@ static int imx6_pcie_probe(struct platform_device
> > *pdev)
> > dev_err(dev, "Failed to get PCIE APPS reset control\n");
> > return PTR_ERR(imx6_pcie->apps_reset);
> > }
> > +
> > + imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
> > + if (IS_ERR(imx6_pcie->pcie_aux)) {
> > + dev_err(dev, "pcie_aux clock source missing or
> > invalid\n");
> > + return PTR_ERR(imx6_pcie->pcie_aux);
> > + }
> > break;
> > default:
> > break;
One can't see enough context in the patch above, but in linux-next this
section is under
case IMX7D:
case IMX8MQ:
It's being applied to imx7d and not just imx8mq and so breaks because
imx7d dts files don't have this clock. Not sure if this is a bug in
this commit or some kind of merge/rebase mistake.
Bug in spi: imx: Add support for SPI Slave mode
On Tue, 2017-09-05 at 14:12 +0900, Jiada Wang wrote:
> Previously i.MX SPI controller only works in Master mode.
> This patch adds support to i.MX51, i.MX53 and i.MX6 ECSPI
> controller to work also in Slave mode.
Recently DMA has been enabled for imx6/7 with SPI. This results in
memory corruption in some instances I've traced the problem to this
patch.
> static int spi_imx_transfer(struct spi_device *spi,
> struct spi_transfer *transfer)
> {
> struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
>
> + /* flush rxfifo before transfer */
> + while (spi_imx->devtype_data->rx_available(spi_imx))
> + spi_imx->rx(spi_imx);
> +
> + if (spi_imx->slave_mode)
> + return spi_imx_pio_transfer_slave(spi, transfer);
> +
> if (spi_imx->usedma)
> return spi_imx_dma_transfer(spi_imx, transfer);
> else
This is in the main xfer function that is used for both master mode and
slave mode. Normally RX data is received after the xfer is started, as
part of the spi_imx_pio/dma_transfer() process. But this patch has
added a "flush rxfifo" command before this. Why?
If it's necessary to empty the fifo before an xfer, then how did this
driver ever work before this change? I see no change anywhere else
that would make this a new requirement.
If the rx fifo is not empty, and this code actually does rx something
from the fifo, then how can it possibly work to place it into the
xfer's RX buffer? How do you know the buffer is large enough (it's not!
that's my DMA bug!)? Won't it offset the actual rx data that comes
after it in the xfer's buffer?
In my test, switching from DMA to PIO, which happens if the 1st xfer is
large enough to pass a >fifo/2 size test, and uses DMA, and the 2nd
xfer is smaller, and will use PIO, results in the PIO xfer trying to
empty the rx buffer in this code above. If there is not enough space
in the xfer's rx buffer, and there is no reason for there to be any
space at all, it clobbers memory.
I suspect the author of this wasn't aware that spi_imx->rx() will write
the data received into the current xfer's rx buffer rather than throw
it away, and never tested this actually getting used for a transfer
where the rx data mattered.
Still, I'd like to know why the flush rx thing is even here. Nothing
in the commit message or any discussion I could find addressed this.
Re: [PATCH v7] PCI: imx6: limit DBI register length
On Mon, 2019-02-25 at 16:15 +, Leonard Crestez wrote:
> On Mon, 2019-02-25 at 17:02 +0100, Stefan Agner wrote:
> > Define the length of the DBI registers and limit config space to its
> > length. This makes sure that the kernel does not access registers
> > beyond that point, avoiding the following abort on a i.MX 6Quad:
> >
> > +static void imx6_pcie_quirk(struct pci_dev *dev)
> > +{
> > + struct pci_bus *bus = dev->bus;
> > + struct pcie_port *pp = bus->sysdata;
> > +
> > + if (bus->number == pp->root_bus_nr) {
> > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > + struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
> > +
> > + /*
> > +* Limit config length to avoid the kernel reading beyond
> > +* the register set and causing an abort on i.MX 6Quad
> > +*/
> > + if (imx6_pcie->drvdata->dbi_length)
> > + dev->cfg_size = imx6_pcie->drvdata->dbi_length;
> > + }
> > +}
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SYNOPSYS, 0xabcd, imx6_pcie_quirk);
>
> This looks like a default from SYNOPSYS so it likely run on other SOCs
> using the DesignWare PCI IP and crash because of those unchecked casts.
Yes, it's used on IMX7d too. But it's worse than that, there's a USB
controller core that uses the same vendor and device id,
PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3. The quirk for that one uses class ==
PCI_CLASS_SERIAL_USB_DEVICE to avoid matching this PCI-e IP. See
thread "PCI: Check for USB xHCI class for HAPS platform"
Re: [PATCH 2/2] ARM: dts: imx7: add DMA properties for ECSPI
On Tue, 2019-02-12 at 22:57 -0200, Fabio Estevam wrote: > On Tue, Feb 12, 2019 at 10:07 PM Trent Piepho > wrote: > > > Tried SDMA firmware 4.2. Still broken. No apparent change. > > > > Get 4 cycle pause after each byte. > > > > And crash while/after using DMA. Clearly some sort of memory > > corruption going on. Fortunately, it's very reliable that using > > DMA > > almost immediately causes a problem and this is easy to > > reproduce. I > > think that indicates it's either clobbers a lot of RAM, or > > consistently > > manages to hit a very important location for kernel memory > > allocators. > > > > I've got an idea of where that might be happening that I'm looking > > into. > > Ok, thanks for investigating this issue. > > > > > I think it's reasonable to add the dma attributes, but put a check > > in > > the spi-imx driver to disable DMA on imx7d at least. > > Something like this? http://dark-code.bulix.org/urfoh8-580174 Something like that. I thought a printk on probe, that DMA was disabled, would be nice so no one beats their head against the wall trying to figure out why DMA isn't being used. But I think I've found the issue and tracked it to bug in the spi core. I'll send a patch shortly. It should affect anything that uses DMA, with a spi master that requires RX and/or TX buffers, and a spi transfer that does not provide the require buffer(s). In my case, spi- imx requires an RX buffer but I am doing TX only DMA. The spi core takes care of this, but I think there is a race in the cleanup of the dummy RX DMA buffer. This appears to clobber something relating to DMA buffer allocation and the kernel starts to allocate bogus DMA buffer addresses, and the SPI controller happily DMAs all over memory. I wonder if that could be somehow exploited to read/write arbitrary memory via SPI DMA?
Re: [PATCH 2/2] ARM: dts: imx7: add DMA properties for ECSPI
On Tue, 2019-02-12 at 17:37 -0200, Fabio Estevam wrote: > On Mon, Feb 11, 2019 at 8:22 PM Trent Piepho > wrote: > > > > Just trying to understand if the SDMA firmware plays a role on > > > this > > > behavior or not. > > > > The ROM firmware only. > > Does the problem also happen if the external SDMA firmware is used? > Just trying to narrow it down. Tried SDMA firmware 4.2. Still broken. No apparent change. Get 4 cycle pause after each byte. And crash while/after using DMA. Clearly some sort of memory corruption going on. Fortunately, it's very reliable that using DMA almost immediately causes a problem and this is easy to reproduce. I think that indicates it's either clobbers a lot of RAM, or consistently manages to hit a very important location for kernel memory allocators. I've got an idea of where that might be happening that I'm looking into. I think it's reasonable to add the dma attributes, but put a check in the spi-imx driver to disable DMA on imx7d at least.
Re: [PATCH 2/2] ARM: dts: imx7: add DMA properties for ECSPI
On Mon, 2019-02-11 at 19:34 -0200, Fabio Estevam wrote: > Hi Trent, > > On Mon, Feb 11, 2019 at 6:44 PM Trent Piepho wrote: > > > I've had more time to test. > > > > Without DMA, I can reload my FPGA hundreds of times and get days of > > uptime using linux-next. > > > > With DMA, loading is unreliable. The higher the SPI speed, the less > > reliable it is. Unfortunately, it's hard to duplicate the problem with > > a small amount of data, and with a large amount of data I can't examine > > it in depth enough to determine if the problem is related to SPI bus > > timing or IMX sDMA sending the wrong bytes to the spi controller. > > > > Using DMA with SPI causes kernel panics. Not always immediately, but > > after using DMA it's a smaller of minutes before something crashes. > > The backtraces are all over the place and don't usually point back into > > SPI. It does seem like they usually hit something has allocated or is > > allocating DMA memory. > > Did you test with an external SDMA firmware or with the ROM SDMA firmware? > > Just trying to understand if the SDMA firmware plays a role on this > behavior or not. The ROM firmware only.
Re: [PATCH 2/2] ARM: dts: imx7: add DMA properties for ECSPI
On Mon, 2019-02-11 at 09:23 +0800, Shawn Guo wrote: > On Thu, Feb 07, 2019 at 09:00:44PM +0000, Trent Piepho wrote: > > On Mon, 2019-01-07 at 14:22 +0100, Stefan Agner wrote: > > > Allow to use DMA for SPI by adding the appropriate DMA properites > > > to the ecspi nodes. > > > > > > Signed-off-by: Stefan Agner > > > > > There's an interesting thing that happens when DMA is used. The SPI > > clock changes. Instead of cycling continuously for the entire > > transfer, it instead clocks out 8 bits, then pauses for 4 bit times, > > then the next byte, etc. So it's a net of about 50% slower. The pause > > between bytes scales with spi frequency to always be about 4 bits. > > > > I think DMA on imx not be ready for prime time yet. > > I dropped both patches from my tree. > I've had more time to test. Without DMA, I can reload my FPGA hundreds of times and get days of uptime using linux-next. With DMA, loading is unreliable. The higher the SPI speed, the less reliable it is. Unfortunately, it's hard to duplicate the problem with a small amount of data, and with a large amount of data I can't examine it in depth enough to determine if the problem is related to SPI bus timing or IMX sDMA sending the wrong bytes to the spi controller. Using DMA with SPI causes kernel panics. Not always immediately, but after using DMA it's a smaller of minutes before something crashes. The backtraces are all over the place and don't usually point back into SPI. It does seem like they usually hit something has allocated or is allocating DMA memory.
Re: [PATCH 2/2] ARM: dts: imx7: add DMA properties for ECSPI
On Mon, 2019-01-07 at 14:22 +0100, Stefan Agner wrote: > Allow to use DMA for SPI by adding the appropriate DMA properites > to the ecspi nodes. > > Signed-off-by: Stefan Agner > --- > arch/arm/boot/dts/imx7s.dtsi | 8 > 1 file changed, 8 insertions(+) > > diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi > index a052198f6e96..b9330176c3af 100644 > --- a/arch/arm/boot/dts/imx7s.dtsi > +++ b/arch/arm/boot/dts/imx7s.dtsi > @@ -653,6 +653,8 @@ > clocks = < IMX7D_ECSPI4_ROOT_CLK>, > < IMX7D_ECSPI4_ROOT_CLK>; > clock-names = "ipg", "per"; > + dmas = < 6 7 1>, < 7 7 2>; > + dma-names = "rx", "tx"; > status = "disabled"; > }; After updating my kernel to linux-next on my IMX7d based device, I found that an FPGA, which is programmed via the ECSPI interface, was no longer accepting its image. I tracked the problem to this change. If I turn off DMA, it works. There's an interesting thing that happens when DMA is used. The SPI clock changes. Instead of cycling continuously for the entire transfer, it instead clocks out 8 bits, then pauses for 4 bit times, then the next byte, etc. So it's a net of about 50% slower. The pause between bytes scales with spi frequency to always be about 4 bits. Here's a trace with DMA: https://imagebin.ca/v/4WEkEnvsVSkq Here's what it looks like without DMA: https://imagebin.ca/v/4WEkVfEqpQ12 It seems like there are other problems with DMA too. Here's an error I'll random get every so often. [ 142.082325] spi_master spi1: I/O Error in DMA RX [ 142.085678] spidev spi1.0: SPI transfer failed: -110 [ 142.089389] spi_master spi1: failed to transfer one message from queue Not sure if the timeout is overly aggressive or if there is some other failure. Then sometimes there errors are worse: Internal error: Oops - undefined instruction: 0 [#1] PREEMPT SMP ARM Modules linked in: CPU: 0 PID: 1006 Comm: fpga-loader Not tainted 5.0.0-rc4-next-20190201 #1 Hardware name: Freescale i.MX7 Dual (Device Tree) PC is at sg_last+0x4c/0x68 LR is at (null) pc : [<80494800>]lr : [<>]psr: 60010013 sp : bf0add5c ip : bea94598 fp : bf371218 r10: bf0949f0 r9 : r8 : bf094800 r7 : bdc39300 r6 : bf371000 r5 : bdc39300 r4 : bf094b68 r3 : r2 : 0001 r1 : 0001 r0 : bea94580 Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user Control: 30c5387d Table: bea94000 DAC: fffd Process fpga-loader (pid: 1006, stack limit = 0x61bc8b73) Stack: (0xbf0add5c to 0xbf0ae000) dd40:8056cdb0 dd60: 8056c9ac 1000 bdc39300 bf094800 bf094b68 bf371000 dd80: bf0949f0 8056bc00 bf094800 bdc39300 bf0adeac bf094800 bf094ad0 80569edc ... dfc0: 00217301 0015 1000 0036 00df2010 0003 1000 00df1008 dfe0: 00022f70 7e9ebb14 000113b8 76f2a7f8 20010030 0003 [<80494800>] (sg_last) from [<8056cdb0>] (spi_imx_transfer+0xd8/0x448) [<8056cdb0>] (spi_imx_transfer) from [<8056bc00>] (spi_bitbang_transfer_one+0x50/0xa0) [<8056bc00>] (spi_bitbang_transfer_one) from [<80569edc>] (spi_transfer_one_message+0x18c/0x3e0) [<80569edc>] (spi_transfer_one_message) from [<8056a498>] (__spi_pump_messages+0x368/0x518) [<8056a498>] (__spi_pump_messages) from [<8056a7ec>] (__spi_sync+0x198/0x1a0) [<8056a7ec>] (__spi_sync) from [<8056a818>] (spi_sync+0x24/0x3c) [<8056a818>] (spi_sync) from [<8056ae14>] (spidev_sync+0x38/0x4c) [<8056ae14>] (spidev_sync) from [<8056b6f4>] (spidev_ioctl+0x660/0x704) [<8056b6f4>] (spidev_ioctl) from [<80335f9c>] (do_vfs_ioctl+0xac/0x79c) [<80335f9c>] (do_vfs_ioctl) from [<803366c0>] (ksys_ioctl+0x34/0x58) [<803366c0>] (ksys_ioctl) from [<80201120>] (ret_fast_syscall+0x0/0x4c) Exception stack(0xbf0adfa8 to 0xbf0adff0) dfa0: 00217301 0015 0003 40206b00 7e9ebb80 03938700 dfc0: 00217301 0015 1000 0036 00df2010 0003 1000 00df1008 dfe0: 00022f70 7e9ebb14 000113b8 76f2a7f8 Code: e1510002 1af3 e353 149df004 (e7f001f2) ---[ end trace 1588229fc7541669 ]--- I think DMA on imx not be ready for prime time yet.
Re: [PATCH v2 8/9] PCI: dwc: Remove Keystone specific dw_pcie_host_ops
On Thu, 2019-02-07 at 16:39 +0530, Kishon Vijay Abraham I wrote:
> Now that Keystone started using it's own msi_irq_chip, remove
> Keystone specific callback function defined in dw_pcie_host_ops.
>
> @@ -209,9 +195,6 @@ static void dw_pci_bottom_ack(struct irq_data *d)
>
> dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit);
>
> - if (pp->ops->msi_irq_ack)
> - pp->ops->msi_irq_ack(d->hwirq, pp);
> -
> raw_spin_unlock_irqrestore(>lock, flags);
> }
After this patch, the entire function now looks like:
dw_pci_bottom_ack(struct irq_data *d)
{
struct pcie_port *pp = irq_data_get_irq_chip_data(d);
unsigned int res, bit, ctrl;
unsigned long flags;
ctrl = d->hwirq / MAX_MSI_IRQS_PER_CTRL;
res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
bit = d->hwirq % MAX_MSI_IRQS_PER_CTRL;
raw_spin_lock_irqsave(>lock, flags);
dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit);
raw_spin_unlock_irqrestore(>lock, flags);
}
The spin lock isn't necessary anymore since there is nothing but a
single register write inside the critical section.
Re: [v6] PCI: imx: make msi work without CONFIG_PCIEPORTBUS=y
On Fri, 2018-12-21 at 04:33 +, Richard Zhu wrote:
> The MSI Enable bit in the MSI Capability (PCIe r4.0, sec 7.7.1.2)
> controls whether a Function can request service using MSI.
>
> i.MX6 Root Ports implement the MSI Capability and may use MSI to
> request service for events like PME, hotplug, AER, etc. In
> addition, on i.MX6, the MSI Enable bit controls delivery of MSI
> interrupts from components below the Root Port.
>
> Prior to commit f3fdfc4ac3a2 ("PCI: Remove host driver Kconfig
> selection
> of CONFIG_PCIEPORTBUS"), enabling CONFIG_PCI_IMX6 automatically also
> enabled CONFIG_PCIEPORTBUS, and when portdrv claimed the Root Ports,
> it set the MSI Enable bit so it could use PME, hotplug, AER, etc.
> As a side effect, that also enabled delivery of MSI interrupts from
> downstream components.
>
> After f3fdfc4ac3a2, the imx6q-pcie driver can operate without
> portdrv, but that means imx6q-pcie must set the MSI Enable bit
> itself if downstream components use MSI.
>
> Fixes: f3fdfc4ac3a2 ("PCI: Remove host driver Kconfig selection of
> CONFIG_PCIEPORTBUS")
>
> Signed-off-by: Richard Zhu
> Reviewed-by: Lucas Stach
> Tested-by: Sven Van Asbroeck
> Acked-by: Lorenzo Pieralisi
Tested on IMX7d, allows MSI to work without enabling PCIEPORTBUS, which
fails without this patch.
Re: [PATCH 09/21] PCI: imx6: Drop imx6_pcie_link_up()
On Thu, 2018-12-20 at 23:27 -0800, Andrey Smirnov wrote:
> Until commit 4d107d3b5a68 ("PCI: imx6: Move link up check into
> imx6_pcie_wait_for_link()") the driver relied on both LINK_UP and
> LINK_IN_TRAINING to determine if PCIE link was up.
I've already got a patch in that fixed this issue. Queued for 4.19 and
4.14 stable.
The problem was created by 886bc5ceb5cc in one branch and by
4d107d3b5a68 on another branch, that when merged in 562df5c8521e, the
merge kept some pieces from each commit and dropped other pieces, which
resulted in this check getting dropped.
Re: [PATCH v3 3/3] PCI: imx6: Add support for i.MX8MQ
On Wed, 2018-12-19 at 16:47 -0800, Andrey Smirnov wrote: > > > > This series initially added explicit offsets but I suggested a single > > > "controller-id" because: > > > * There are multiple bit and byte offsets > > > * Other imx8 SOCs also have 2x pcie with other bit/byte offsets > > > > > > Hiding this behind a compatible string and single "controller-id" seem > > > preferable to elaborating register maps in dt bindings. It also makes > > > upgrades simpler: if features are added which use other bits there is no > > > need to describe them in DT and deal with compatibility headaches. > > > > You already have an id for the controllers: the address. Use that if > > you don't want to put the register offsets in DT. > > > > Lucas, are you on board with this? Does address here mean the address from the controller's reg property? How do you map that address to the controller's index? A giant table of every soc so the soc type plus controller register address pair than can be looked up in the driver? I.e., on iMX8MQ the controller at 0x3380 is controller 0 and so on for every possible SoC address combination? Not really a fan of that. The situation here is that some registers for these controllers are interleaved, right? I.e., there's one register somewhere where bit 0 means enable controller 0 and bit 1 means enable controller 1 and so on. Isn't cell-index already the standard device tree property for this kind of setup? I know cell-index was historically also (ab)used in an attempt to provide a fixed kernel device enumeration order, something now handled better by chosen node aliases.
Re: [RFC BUG] pci: Freescale i.MX6 PCIe controller: revert mainline regression
On Tue, 2018-12-18 at 16:04 -0500, Sven Van Asbroeck wrote:
> We are using a Broadcom BCM57780 ethernet adapter (Tigon3) connected
> to a i.MX6 PCIe controller. On a Freescale i.MX6 SoC.
>
> This combo worked ok on mainline v4.17.
> On mainline v4.18-rc1, a regression was introduced that caused a crash.
> This regression is still present in current mainline as of v4.20-rc7
>
> Bisected the regression to the following merge head:
> commit 0ecda3a08746 ("Merge branch 'pci/kconfig'")
> When this merge head is removed, the regression disappears. This commit
> reverts the changes introduced by the offending merge head.
It's actually commit f3fdfc4ac3a2 ("PCI: Remove host driver Kconfig
selection of CONFIG_PCIEPORTBUS") that made this problem apparent.
But the real bug is not in that commit, but a problem with imx6/7 not
configuring a setting in the RC correctly without pcieportbus, which
should not be necessary.
See "[v4] PCI: imx: make msi work without CONFIG_PCIEPORTBUS=y" by
Richard Zhu for a real fix.
There another bug that makes this driver sometimes lose an MSI, but
that regression was in v4.14 and would not be fixed by your patch, so
this tigon3 problem is almost certainly caused by the previously
mentioned issue with pcieportbus.
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index 6671946dbf66..6e4241ac0a1e 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -113,6 +113,7 @@ config PCI_XGENE
> bool "X-Gene PCIe controller"
> depends on ARM64 || COMPILE_TEST
> depends on OF || (ACPI && PCI_QUIRKS)
> + select PCIEPORTBUS
> help
> Say Y here if you want internal PCI support on APM X-Gene SoC.
>
Re: [PATCH] PCI: controller: dwc: Make PCI_IMX6 depend on PCIEPORTBUS
On Thu, 2018-12-06 at 11:28 +0100, Lucas Stach wrote: > Am Mittwoch, den 05.12.2018, 23:45 -0800 schrieb Andrey Smirnov: > > Building a kernel with CONFIG_PCI_IMX6=y, but CONFIG_PCIEPORTBUS=n > > produces a system where built-in PCIE bridge (16c3:abcd) isn't > > bound > > to pcieport driver. This, in turn, results in a PCIE bus that is > > capable of enumerating attached PCIE device, but lacks functional > > interrupt support. > > This is odd. AFAIK PCI port services are a totally optional thing and > them being absent should not lead to a non-functional PCI bus. So I > would really like to see some deeper analysis what is going on here. I noticed this previously, https://www.spinics.net/lists/linux-pci/msg77840.html What happened was that PCIEPORTBUS *was* selected by all the platform dwc drivers and those selections were removed in: commit f3fdfc4ac3a26865e84627a7fe08a906081e5abc Author: Bjorn Helgaas Date: Fri May 18 15:08:36 2018 -0500 PCI: Remove host driver Kconfig selection of CONFIG_PCIEPORTBUS Host bridge drivers do not use the portdrv interfaces (struct pcie_device, struct pcie_port_service_driver, pcie_port_service_register(), etc), and they should not select CONFIG_PCIEPORTBUS. I encountered this when a kernel defconfig that previously turned on PCIEPORTBUS stopped turning in on, resulting in the situation that Andrey describes: no errors, device present, but no MSI interrupts. The in kernel defconfigs (mine not being one of those) for all the dwc pcie users should probably be refreshed to enable pcieportbus. It seems appropriate that the port services are optional and it's unfortunate that the kernel defconfig system resolves in the change in this manner, vs keeping pcieportbus on by default. I do know why it is this way and I don't think there is any great solution there. It has never been clear to me why pcieportbus was necessary for MSI.
Re: [PATCH] PCI: controller: dwc: Make PCI_IMX6 depend on PCIEPORTBUS
On Thu, 2018-12-06 at 11:28 +0100, Lucas Stach wrote: > Am Mittwoch, den 05.12.2018, 23:45 -0800 schrieb Andrey Smirnov: > > Building a kernel with CONFIG_PCI_IMX6=y, but CONFIG_PCIEPORTBUS=n > > produces a system where built-in PCIE bridge (16c3:abcd) isn't > > bound > > to pcieport driver. This, in turn, results in a PCIE bus that is > > capable of enumerating attached PCIE device, but lacks functional > > interrupt support. > > This is odd. AFAIK PCI port services are a totally optional thing and > them being absent should not lead to a non-functional PCI bus. So I > would really like to see some deeper analysis what is going on here. I noticed this previously, https://www.spinics.net/lists/linux-pci/msg77840.html What happened was that PCIEPORTBUS *was* selected by all the platform dwc drivers and those selections were removed in: commit f3fdfc4ac3a26865e84627a7fe08a906081e5abc Author: Bjorn Helgaas Date: Fri May 18 15:08:36 2018 -0500 PCI: Remove host driver Kconfig selection of CONFIG_PCIEPORTBUS Host bridge drivers do not use the portdrv interfaces (struct pcie_device, struct pcie_port_service_driver, pcie_port_service_register(), etc), and they should not select CONFIG_PCIEPORTBUS. I encountered this when a kernel defconfig that previously turned on PCIEPORTBUS stopped turning in on, resulting in the situation that Andrey describes: no errors, device present, but no MSI interrupts. The in kernel defconfigs (mine not being one of those) for all the dwc pcie users should probably be refreshed to enable pcieportbus. It seems appropriate that the port services are optional and it's unfortunate that the kernel defconfig system resolves in the change in this manner, vs keeping pcieportbus on by default. I do know why it is this way and I don't think there is any great solution there. It has never been clear to me why pcieportbus was necessary for MSI.
Re: [PATCH] pci: imx6: support kernels built in Thumb-2 mode
On Wed, 2018-11-28 at 19:52 +, Robin Murphy wrote:
> On 28/11/2018 18:56, Trent Piepho wrote:
> > On Wed, 2018-11-28 at 16:16 +, Robin Murphy wrote:
> > >
> > > >
> > > > +static int imx6q_pcie_abort_handler_thumb2(unsigned long addr,
> > > > + unsigned int fsr, struct pt_regs *regs)
> > > > +{
> > > > + unsigned long pc = instruction_pointer(regs);
> > > > + unsigned long instr = *(unsigned long *)pc;
> > > > + unsigned long thumb2_instr =
> > > > __mem_to_opcode_thumb16(instr);
> > > > + int reg = thumb2_instr & 7;
> > > > +
> > > > + if (!__opcode_is_thumb16(instr & 0xUL))
> > > > + return 1;
> > >
> > > There are plenty of 32-bit Thumb encodings of various LDR/STR
> > > variants,
> > > and I doubt we can guarantee that the offset, target register,
> > > and/or
> > > addressing mode for a config space access will *always* suit the
> > > (relatively limited) 16-bit ones.
> >
> > It might be the case that PLD/PLI, 32-bit thumb2 instructions,
> > could
> > trigger an abort too.
>
> Preload instructions shouldn't cause a *synchronous* abort, which is
> what we're trapping here, and they could only result in an asynchronous
> abort coming back later if the address is mapped as Normal memory, which
> it really shouldn't be in this case. Frankly either way, anyone even
> thinking about trying to pull PCI config space into data caches, let
> alone instruction caches, probably deserves everything they get ;)
It would be easy to do from userspace. mmap the config space attribute
in sysfs, and then call plain old glibc memcpy() on it to copy to
normal ram. strlen(), memcpy(), etc. are coded with pld instructions.
Re: [PATCH] pci: imx6: support kernels built in Thumb-2 mode
On Wed, 2018-11-28 at 19:52 +, Robin Murphy wrote:
> On 28/11/2018 18:56, Trent Piepho wrote:
> > On Wed, 2018-11-28 at 16:16 +, Robin Murphy wrote:
> > >
> > > >
> > > > +static int imx6q_pcie_abort_handler_thumb2(unsigned long addr,
> > > > + unsigned int fsr, struct pt_regs *regs)
> > > > +{
> > > > + unsigned long pc = instruction_pointer(regs);
> > > > + unsigned long instr = *(unsigned long *)pc;
> > > > + unsigned long thumb2_instr =
> > > > __mem_to_opcode_thumb16(instr);
> > > > + int reg = thumb2_instr & 7;
> > > > +
> > > > + if (!__opcode_is_thumb16(instr & 0xUL))
> > > > + return 1;
> > >
> > > There are plenty of 32-bit Thumb encodings of various LDR/STR
> > > variants,
> > > and I doubt we can guarantee that the offset, target register,
> > > and/or
> > > addressing mode for a config space access will *always* suit the
> > > (relatively limited) 16-bit ones.
> >
> > It might be the case that PLD/PLI, 32-bit thumb2 instructions,
> > could
> > trigger an abort too.
>
> Preload instructions shouldn't cause a *synchronous* abort, which is
> what we're trapping here, and they could only result in an asynchronous
> abort coming back later if the address is mapped as Normal memory, which
> it really shouldn't be in this case. Frankly either way, anyone even
> thinking about trying to pull PCI config space into data caches, let
> alone instruction caches, probably deserves everything they get ;)
It would be easy to do from userspace. mmap the config space attribute
in sysfs, and then call plain old glibc memcpy() on it to copy to
normal ram. strlen(), memcpy(), etc. are coded with pld instructions.
Re: [PATCH] pci: imx6: support kernels built in Thumb-2 mode
On Wed, 2018-11-28 at 16:16 +, Robin Murphy wrote:
>
> >
> > +static int imx6q_pcie_abort_handler_thumb2(unsigned long addr,
> > + unsigned int fsr, struct pt_regs *regs)
> > +{
> > + unsigned long pc = instruction_pointer(regs);
> > + unsigned long instr = *(unsigned long *)pc;
> > + unsigned long thumb2_instr = __mem_to_opcode_thumb16(instr);
> > + int reg = thumb2_instr & 7;
> > +
> > + if (!__opcode_is_thumb16(instr & 0xUL))
> > + return 1;
>
> There are plenty of 32-bit Thumb encodings of various LDR/STR variants,
> and I doubt we can guarantee that the offset, target register, and/or
> addressing mode for a config space access will *always* suit the
> (relatively limited) 16-bit ones.
It might be the case that PLD/PLI, 32-bit thumb2 instructions, could
trigger an abort too.
Re: [PATCH] pci: imx6: support kernels built in Thumb-2 mode
On Wed, 2018-11-28 at 16:16 +, Robin Murphy wrote:
>
> >
> > +static int imx6q_pcie_abort_handler_thumb2(unsigned long addr,
> > + unsigned int fsr, struct pt_regs *regs)
> > +{
> > + unsigned long pc = instruction_pointer(regs);
> > + unsigned long instr = *(unsigned long *)pc;
> > + unsigned long thumb2_instr = __mem_to_opcode_thumb16(instr);
> > + int reg = thumb2_instr & 7;
> > +
> > + if (!__opcode_is_thumb16(instr & 0xUL))
> > + return 1;
>
> There are plenty of 32-bit Thumb encodings of various LDR/STR variants,
> and I doubt we can guarantee that the offset, target register, and/or
> addressing mode for a config space access will *always* suit the
> (relatively limited) 16-bit ones.
It might be the case that PLD/PLI, 32-bit thumb2 instructions, could
trigger an abort too.
Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length
On Mon, 2018-11-26 at 10:16 +, Leonard Crestez wrote:
> On Tue, 2018-11-20 at 21:42 +0100, Stefan Agner wrote:
> > On 20.11.2018 20:13, Trent Piepho wrote:
> > > On Tue, 2018-11-20 at 18:19 +, Leonard Crestez wrote:
> > > > On Tue, 2018-11-20 at 17:56 +0100, Stefan Agner wrote:
> > > > > Define the length of the DBI registers. This makes sure that
> > > > > the kernel does not access registers beyond that point, avoiding
> > > > > the following abort on a i.MX 6Quad:
> > > > > # cat
> > > > > /sys/devices/soc0/soc/1ffc000.pcie/pci\:00/\:00\:00.0/config
> > > > > [ 100.021433] Unhandled fault: imprecise external abort (0x1406)
> > > > > at 0xb6ea7000
> > > > > ...
> > > > > [ 100.056423] PC is at dw_pcie_read+0x50/0x84
> > > > > [ 100.060790] LR is at dw_pcie_rd_own_conf+0x44/0x48
> > > >
> > > > I don't know exactly where this limitation comes from, I can indeed
> > > > reproduce a stack dump when dumping pci config from /sys/
> > > >
> > > > Unfortunately this seems to block access to registers used for
> > > > functionality like interrupts. For example dw_handle_msi_irq does:
> > > >
> > > > dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS +
> > > > (i * MSI_REG_CTRL_BLOCK_SIZE),
> > > > 4, );
> > > >
> > > > where PCI_MSI_INTR0_STATUS is 0x830. There are more accesses like this.
> > >
> > > On IMX7d, there are significant blocks of 00s in the config space, and
> > > all 0xff at 0xb50 on up.
> > >
> > > I.e., significant portions are empty, in the middle of the config
> > > space, not just at the end.
> > >
> > > But they can be read without problem.
> > >
> > > Perhaps imx6q aborts on a read of an unimplemented address instead of
> > > returning zeros like imx7d. In that case it really needs something
> > > more complex to prevent abort than just a length.
> >
> > Yeah it seems those SoCs behave differently.
> >
> > Describing a register set with holes will get complicated, I guess it
> > would ask for a regmap...
>
> The PortLogic area seems to be always at 0x700-0xA00, there are defines
> for it. So this would only require one additional range, no regmap
> stuff.
>
> I don't know if making portlogic accessible to userspace is useful. In
> theory somebody could read debug DWC registers via /sys/blah/config,
> but there are better ways to read HW registers from userspace.
>
> Browsing through the docs I can't find stuff like reads with side-
> effects so I can't say that it's harmful either.
I doubt those register are used much from userspace, since AFAIK there
are no public docs for them. I have dumped the config regs from
userspace in imx7d to debug a hang problem, but those registers weren't
useful to me. Maybe they would have been, no docs...
> > > It also seems to me that this doesn't need to be in the internal pci
> > > config access functions. The driver shouldn't be reading registers
> > > that don't exist anyway. It's really about trying to fix sysfs access
> > > to registers that don't exist. So maybe it should be done there.
> >
> > That was my first approach
>
> Doing it on a per-soc basis is better, this seems to affect both 6q and
> 6qp (those are distinct!) but not others.
>
> The pci config area from userspace is accessed through pci_ops.read =
> dw_pcie_rd_conf while internal accesses go through dw_pcie_rd_own_conf.
> So moving the dbi_length check up one level would work easily.
What about something like:
static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
int size, u32 *val)
{
struct pcie_port *pp = bus->sysdata;
if (!dw_pcie_valid_device(pp, bus, PCI_SLOT(devfn))) {
*val = 0x;
return PCIBIOS_DEVICE_NOT_FOUND;
}
if (bus->number == pp->root_bus_nr) {
+ if (pp->ops->cfg_valid && !pp->ops_cfg_valid(pp, where, size)) {
+ *val = 0x;
+ return PCIBIOS_SOMETHING_GOES_HERE;
+ }
+
return dw_pcie_rd_own_conf(pp, where, size, val);
+ }
return dw_pcie_rd_other_conf(pp, bus, devfn, where, size, val);
}
imx6q and imx6qp can provide cfg_valid that checks their range.
Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length
On Mon, 2018-11-26 at 10:16 +, Leonard Crestez wrote:
> On Tue, 2018-11-20 at 21:42 +0100, Stefan Agner wrote:
> > On 20.11.2018 20:13, Trent Piepho wrote:
> > > On Tue, 2018-11-20 at 18:19 +, Leonard Crestez wrote:
> > > > On Tue, 2018-11-20 at 17:56 +0100, Stefan Agner wrote:
> > > > > Define the length of the DBI registers. This makes sure that
> > > > > the kernel does not access registers beyond that point, avoiding
> > > > > the following abort on a i.MX 6Quad:
> > > > > # cat
> > > > > /sys/devices/soc0/soc/1ffc000.pcie/pci\:00/\:00\:00.0/config
> > > > > [ 100.021433] Unhandled fault: imprecise external abort (0x1406)
> > > > > at 0xb6ea7000
> > > > > ...
> > > > > [ 100.056423] PC is at dw_pcie_read+0x50/0x84
> > > > > [ 100.060790] LR is at dw_pcie_rd_own_conf+0x44/0x48
> > > >
> > > > I don't know exactly where this limitation comes from, I can indeed
> > > > reproduce a stack dump when dumping pci config from /sys/
> > > >
> > > > Unfortunately this seems to block access to registers used for
> > > > functionality like interrupts. For example dw_handle_msi_irq does:
> > > >
> > > > dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS +
> > > > (i * MSI_REG_CTRL_BLOCK_SIZE),
> > > > 4, );
> > > >
> > > > where PCI_MSI_INTR0_STATUS is 0x830. There are more accesses like this.
> > >
> > > On IMX7d, there are significant blocks of 00s in the config space, and
> > > all 0xff at 0xb50 on up.
> > >
> > > I.e., significant portions are empty, in the middle of the config
> > > space, not just at the end.
> > >
> > > But they can be read without problem.
> > >
> > > Perhaps imx6q aborts on a read of an unimplemented address instead of
> > > returning zeros like imx7d. In that case it really needs something
> > > more complex to prevent abort than just a length.
> >
> > Yeah it seems those SoCs behave differently.
> >
> > Describing a register set with holes will get complicated, I guess it
> > would ask for a regmap...
>
> The PortLogic area seems to be always at 0x700-0xA00, there are defines
> for it. So this would only require one additional range, no regmap
> stuff.
>
> I don't know if making portlogic accessible to userspace is useful. In
> theory somebody could read debug DWC registers via /sys/blah/config,
> but there are better ways to read HW registers from userspace.
>
> Browsing through the docs I can't find stuff like reads with side-
> effects so I can't say that it's harmful either.
I doubt those register are used much from userspace, since AFAIK there
are no public docs for them. I have dumped the config regs from
userspace in imx7d to debug a hang problem, but those registers weren't
useful to me. Maybe they would have been, no docs...
> > > It also seems to me that this doesn't need to be in the internal pci
> > > config access functions. The driver shouldn't be reading registers
> > > that don't exist anyway. It's really about trying to fix sysfs access
> > > to registers that don't exist. So maybe it should be done there.
> >
> > That was my first approach
>
> Doing it on a per-soc basis is better, this seems to affect both 6q and
> 6qp (those are distinct!) but not others.
>
> The pci config area from userspace is accessed through pci_ops.read =
> dw_pcie_rd_conf while internal accesses go through dw_pcie_rd_own_conf.
> So moving the dbi_length check up one level would work easily.
What about something like:
static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
int size, u32 *val)
{
struct pcie_port *pp = bus->sysdata;
if (!dw_pcie_valid_device(pp, bus, PCI_SLOT(devfn))) {
*val = 0x;
return PCIBIOS_DEVICE_NOT_FOUND;
}
if (bus->number == pp->root_bus_nr) {
+ if (pp->ops->cfg_valid && !pp->ops_cfg_valid(pp, where, size)) {
+ *val = 0x;
+ return PCIBIOS_SOMETHING_GOES_HERE;
+ }
+
return dw_pcie_rd_own_conf(pp, where, size, val);
+ }
return dw_pcie_rd_other_conf(pp, bus, devfn, where, size, val);
}
imx6q and imx6qp can provide cfg_valid that checks their range.
Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length
On Tue, 2018-11-20 at 21:42 +0100, Stefan Agner wrote: > On 20.11.2018 20:13, Trent Piepho wrote: > > > > On IMX7d, there are significant blocks of 00s in the config space, and > > all 0xff at 0xb50 on up. > > > > I.e., significant portions are empty, in the middle of the config > > space, not just at the end. > > > > But they can be read without problem. > > > > Perhaps imx6q aborts on a read of an unimplemented address instead of > > returning zeros like imx7d. In that case it really needs something > > more complex to prevent abort than just a length. > > Yeah it seems those SoCs behave differently. > > Describing a register set with holes will get complicated, I guess it > would ask for a regmap... > > > > > It also seems to me that this doesn't need to be in the internal pci > > config access functions. The driver shouldn't be reading registers > > that don't exist anyway. It's really about trying to fix sysfs access > > to registers that don't exist. So maybe it should be done there. > > That was my first approach, see: https://lkml.org/lkml/2018/11/14/716 Yes, but that just used the pci device id which applies to every IMX design. It's also not totally correct, as it seems real registers after 0x200 do work on imx6, and that would prevent access to them. Like you say, it could use a regmap. Seems kinda overkill to me though. I wonder if regmap based caching of register to avoid RMW cycles would be generally useful? I know the enable and mask registers are/were cached in the driver (irq_state[]).
Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length
On Tue, 2018-11-20 at 21:42 +0100, Stefan Agner wrote: > On 20.11.2018 20:13, Trent Piepho wrote: > > > > On IMX7d, there are significant blocks of 00s in the config space, and > > all 0xff at 0xb50 on up. > > > > I.e., significant portions are empty, in the middle of the config > > space, not just at the end. > > > > But they can be read without problem. > > > > Perhaps imx6q aborts on a read of an unimplemented address instead of > > returning zeros like imx7d. In that case it really needs something > > more complex to prevent abort than just a length. > > Yeah it seems those SoCs behave differently. > > Describing a register set with holes will get complicated, I guess it > would ask for a regmap... > > > > > It also seems to me that this doesn't need to be in the internal pci > > config access functions. The driver shouldn't be reading registers > > that don't exist anyway. It's really about trying to fix sysfs access > > to registers that don't exist. So maybe it should be done there. > > That was my first approach, see: https://lkml.org/lkml/2018/11/14/716 Yes, but that just used the pci device id which applies to every IMX design. It's also not totally correct, as it seems real registers after 0x200 do work on imx6, and that would prevent access to them. Like you say, it could use a regmap. Seems kinda overkill to me though. I wonder if regmap based caching of register to avoid RMW cycles would be generally useful? I know the enable and mask registers are/were cached in the driver (irq_state[]).
Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length
On Tue, 2018-11-20 at 18:19 +, Leonard Crestez wrote: > On Tue, 2018-11-20 at 17:56 +0100, Stefan Agner wrote: > > Define the length of the DBI registers. This makes sure that > > the kernel does not access registers beyond that point, avoiding > > the following abort on a i.MX 6Quad: > > # cat > > /sys/devices/soc0/soc/1ffc000.pcie/pci\:00/\:00\:00.0/config > > [ 100.021433] Unhandled fault: imprecise external abort (0x1406) > > at 0xb6ea7000 > > ... > > [ 100.056423] PC is at dw_pcie_read+0x50/0x84 > > [ 100.060790] LR is at dw_pcie_rd_own_conf+0x44/0x48 > > I don't know exactly where this limitation comes from, I can indeed > reproduce a stack dump when dumping pci config from /sys/ > > Unfortunately this seems to block access to registers used for > functionality like interrupts. For example dw_handle_msi_irq does: > > dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS + > (i * MSI_REG_CTRL_BLOCK_SIZE), > 4, ); > > where PCI_MSI_INTR0_STATUS is 0x830. There are more accesses like this. > > Testing on 6dl-sabreauto (dts change required) with an ath9k pcie card > with your series I sometimes get "irq 295: nobody cared" on boot. Maybe > I'm missing something? On IMX7d, there are significant blocks of 00s in the config space, and all 0xff at 0xb50 on up. I.e., significant portions are empty, in the middle of the config space, not just at the end. But they can be read without problem. Perhaps imx6q aborts on a read of an unimplemented address instead of returning zeros like imx7d. In that case it really needs something more complex to prevent abort than just a length. It also seems to me that this doesn't need to be in the internal pci config access functions. The driver shouldn't be reading registers that don't exist anyway. It's really about trying to fix sysfs access to registers that don't exist. So maybe it should be done there.
Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length
On Tue, 2018-11-20 at 18:19 +, Leonard Crestez wrote: > On Tue, 2018-11-20 at 17:56 +0100, Stefan Agner wrote: > > Define the length of the DBI registers. This makes sure that > > the kernel does not access registers beyond that point, avoiding > > the following abort on a i.MX 6Quad: > > # cat > > /sys/devices/soc0/soc/1ffc000.pcie/pci\:00/\:00\:00.0/config > > [ 100.021433] Unhandled fault: imprecise external abort (0x1406) > > at 0xb6ea7000 > > ... > > [ 100.056423] PC is at dw_pcie_read+0x50/0x84 > > [ 100.060790] LR is at dw_pcie_rd_own_conf+0x44/0x48 > > I don't know exactly where this limitation comes from, I can indeed > reproduce a stack dump when dumping pci config from /sys/ > > Unfortunately this seems to block access to registers used for > functionality like interrupts. For example dw_handle_msi_irq does: > > dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS + > (i * MSI_REG_CTRL_BLOCK_SIZE), > 4, ); > > where PCI_MSI_INTR0_STATUS is 0x830. There are more accesses like this. > > Testing on 6dl-sabreauto (dts change required) with an ath9k pcie card > with your series I sometimes get "irq 295: nobody cared" on boot. Maybe > I'm missing something? On IMX7d, there are significant blocks of 00s in the config space, and all 0xff at 0xb50 on up. I.e., significant portions are empty, in the middle of the config space, not just at the end. But they can be read without problem. Perhaps imx6q aborts on a read of an unimplemented address instead of returning zeros like imx7d. In that case it really needs something more complex to prevent abort than just a length. It also seems to me that this doesn't need to be in the internal pci config access functions. The driver shouldn't be reading registers that don't exist anyway. It's really about trying to fix sysfs access to registers that don't exist. So maybe it should be done there.
Re: [PATCH 2/2] PCI: imx6: limit DBI register length
On Mon, 2018-11-19 at 10:41 +0100, Stefan Agner wrote:
>
>
> +static const struct imx6_pcie_drvdata imx6q_pcie_drvdata = {
> + .variant = IMX6Q,
> + .dbi_length = 0x15c,
> +};
> +
> +static const struct imx6_pcie_drvdata imx6sx_pcie_drvdata = {
> + .variant = IMX6SX,
> +};
> +
> +static const struct imx6_pcie_drvdata imx6qp_pcie_drvdata = {
> + .variant = IMX6QP,
> +};
> +
> +static const struct imx6_pcie_drvdata imx7d_pcie_drvdata = {
> + .variant = IMX7D,
> +};
> +
> static const struct of_device_id imx6_pcie_of_match[] = {
> - { .compatible = "fsl,imx6q-pcie", .data = (void *)IMX6Q, },
> - { .compatible = "fsl,imx6sx-pcie", .data = (void *)IMX6SX, },
> - { .compatible = "fsl,imx6qp-pcie", .data = (void *)IMX6QP, },
> - { .compatible = "fsl,imx7d-pcie", .data = (void *)IMX7D, },
> + { .compatible = "fsl,imx6q-pcie", .data = _pcie_drvdata, },
> + { .compatible = "fsl,imx6sx-pcie", .data = _pcie_drvdata, },
> + { .compatible = "fsl,imx6qp-pcie", .data = _pcie_drvdata, },
> + { .compatible = "fsl,imx7d-pcie", .data = _pcie_drvdata, },
> {},
> };
Instead of making a single drvdata struct for each type, this could use
an array:
static const struct imx6_pcie_drvdata drvdata[] = {
[IMX6Q] = { .variant = IMX6Q, .dbi_length = 0x15c },
[IMX6SX] = { .variant = IMX6SX },
[...]
};
static const struct of_device_id imx6_pcie_of_match[] = {
{ .compatible = "fsl,imx6q-pcie", .data = [IMX6Q], },
{ .compatible = "fsl,imx6sx-pcie", .data = [IMX6SX], },
...
};
Re: [PATCH 2/2] PCI: imx6: limit DBI register length
On Mon, 2018-11-19 at 10:41 +0100, Stefan Agner wrote:
>
>
> +static const struct imx6_pcie_drvdata imx6q_pcie_drvdata = {
> + .variant = IMX6Q,
> + .dbi_length = 0x15c,
> +};
> +
> +static const struct imx6_pcie_drvdata imx6sx_pcie_drvdata = {
> + .variant = IMX6SX,
> +};
> +
> +static const struct imx6_pcie_drvdata imx6qp_pcie_drvdata = {
> + .variant = IMX6QP,
> +};
> +
> +static const struct imx6_pcie_drvdata imx7d_pcie_drvdata = {
> + .variant = IMX7D,
> +};
> +
> static const struct of_device_id imx6_pcie_of_match[] = {
> - { .compatible = "fsl,imx6q-pcie", .data = (void *)IMX6Q, },
> - { .compatible = "fsl,imx6sx-pcie", .data = (void *)IMX6SX, },
> - { .compatible = "fsl,imx6qp-pcie", .data = (void *)IMX6QP, },
> - { .compatible = "fsl,imx7d-pcie", .data = (void *)IMX7D, },
> + { .compatible = "fsl,imx6q-pcie", .data = _pcie_drvdata, },
> + { .compatible = "fsl,imx6sx-pcie", .data = _pcie_drvdata, },
> + { .compatible = "fsl,imx6qp-pcie", .data = _pcie_drvdata, },
> + { .compatible = "fsl,imx7d-pcie", .data = _pcie_drvdata, },
> {},
> };
Instead of making a single drvdata struct for each type, this could use
an array:
static const struct imx6_pcie_drvdata drvdata[] = {
[IMX6Q] = { .variant = IMX6Q, .dbi_length = 0x15c },
[IMX6SX] = { .variant = IMX6SX },
[...]
};
static const struct of_device_id imx6_pcie_of_match[] = {
{ .compatible = "fsl,imx6q-pcie", .data = [IMX6Q], },
{ .compatible = "fsl,imx6sx-pcie", .data = [IMX6SX], },
...
};
Re: [PATCH 2/3] PCI: imx: No-op imx6_pcie_reset_phy() on i.MX7D
On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote: > PCIE PHY IP block on i.MX7D differs from the one used on i.MX6 family, > so none of the code in current implementation of imx6_pcie_reset_phy() > is applicable. Tested on IMX7d, still appears to work. Note that your patches will collide with Stefan Agner's patch, "PCI: imx6: limit DBI register length", which was recently posted. He changed the way the variants are handled. That method would allow some of the IMX7D || IMX8MQ checks to be re-written as imx6_pcie->drvdata->boolean_attribute Where the attribute can be set in a table and be re-used in every place it comes into play and updated for new devices in one spot, instead of keeping piles of this version or that version or this other version checks up to date.
Re: [PATCH 2/3] PCI: imx: No-op imx6_pcie_reset_phy() on i.MX7D
On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote: > PCIE PHY IP block on i.MX7D differs from the one used on i.MX6 family, > so none of the code in current implementation of imx6_pcie_reset_phy() > is applicable. Tested on IMX7d, still appears to work. Note that your patches will collide with Stefan Agner's patch, "PCI: imx6: limit DBI register length", which was recently posted. He changed the way the variants are handled. That method would allow some of the IMX7D || IMX8MQ checks to be re-written as imx6_pcie->drvdata->boolean_attribute Where the attribute can be set in a table and be re-used in every place it comes into play and updated for new devices in one spot, instead of keeping piles of this version or that version or this other version checks up to date.
Re: [PATCH 2/2] PCI: imx6: limit DBI register length
On Mon, 2018-11-19 at 10:41 +0100, Stefan Agner wrote: > Define the length of the DBI registers. This makes sure that > the kernel does not access registers beyond that point, avoiding > the following abort on a i.MX 6Quad: > # cat > /sys/devices/soc0/soc/1ffc000.pcie/pci\:00/\:00\:00.0/config > [ 100.021433] Unhandled fault: imprecise external abort (0x1406) > at 0xb6ea7000 > ... > [ 100.056423] PC is at dw_pcie_read+0x50/0x84 > [ 100.060790] LR is at dw_pcie_rd_own_conf+0x44/0x48 > ... > > Signed-off-by: Stefan Agner After updating this for v4.20-rc2, tested on IMX 7d, config space access works as before. Tested-by: Trent Piepho
Re: [PATCH 2/2] PCI: imx6: limit DBI register length
On Mon, 2018-11-19 at 10:41 +0100, Stefan Agner wrote: > Define the length of the DBI registers. This makes sure that > the kernel does not access registers beyond that point, avoiding > the following abort on a i.MX 6Quad: > # cat > /sys/devices/soc0/soc/1ffc000.pcie/pci\:00/\:00\:00.0/config > [ 100.021433] Unhandled fault: imprecise external abort (0x1406) > at 0xb6ea7000 > ... > [ 100.056423] PC is at dw_pcie_read+0x50/0x84 > [ 100.060790] LR is at dw_pcie_rd_own_conf+0x44/0x48 > ... > > Signed-off-by: Stefan Agner After updating this for v4.20-rc2, tested on IMX 7d, config space access works as before. Tested-by: Trent Piepho
[PATCH v2] PCI: dwc: Adjust Kconfig to allow IMX6 PCIe host on IMX7
The IMX6 PCI-e host drier also supports the IMX7d. However, the Kconfig dependencies of the driver prevented it from being enabled unless the kernel was built with both IMX6 and IMX7 support. It works fine to build with only IMX7 support enabled. Cc: Andrey Smirnov Reviewed-by: Lucas Stach Signed-off-by: Trent Piepho --- Changes from v1: Ported to current kernel drivers/pci/controller/dwc/Kconfig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig index 91b0194240a5..6aafec3fad00 100644 --- a/drivers/pci/controller/dwc/Kconfig +++ b/drivers/pci/controller/dwc/Kconfig @@ -89,8 +89,8 @@ config PCI_EXYNOS select PCIE_DW_HOST config PCI_IMX6 - bool "Freescale i.MX6 PCIe controller" - depends on SOC_IMX6Q || (ARM && COMPILE_TEST) + bool "Freescale i.MX6/7 PCIe controller" + depends on SOC_IMX6Q || SOC_IMX7D || (ARM && COMPILE_TEST) depends on PCI_MSI_IRQ_DOMAIN select PCIE_DW_HOST -- 2.14.4
[PATCH v2] PCI: dwc: Adjust Kconfig to allow IMX6 PCIe host on IMX7
The IMX6 PCI-e host drier also supports the IMX7d. However, the Kconfig dependencies of the driver prevented it from being enabled unless the kernel was built with both IMX6 and IMX7 support. It works fine to build with only IMX7 support enabled. Cc: Andrey Smirnov Reviewed-by: Lucas Stach Signed-off-by: Trent Piepho --- Changes from v1: Ported to current kernel drivers/pci/controller/dwc/Kconfig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig index 91b0194240a5..6aafec3fad00 100644 --- a/drivers/pci/controller/dwc/Kconfig +++ b/drivers/pci/controller/dwc/Kconfig @@ -89,8 +89,8 @@ config PCI_EXYNOS select PCIE_DW_HOST config PCI_IMX6 - bool "Freescale i.MX6 PCIe controller" - depends on SOC_IMX6Q || (ARM && COMPILE_TEST) + bool "Freescale i.MX6/7 PCIe controller" + depends on SOC_IMX6Q || SOC_IMX7D || (ARM && COMPILE_TEST) depends on PCI_MSI_IRQ_DOMAIN select PCIE_DW_HOST -- 2.14.4
[PATCH] rtc: isl1208: Use i2c block read/write routines
The Linux i2c layer has functions to execute common SMBUS/I2C
transactions. The register access code here is identical to the I2C
read/write block data routines.
Cc: Alessandro Zummo
Cc: Alexandre Belloni
Signed-off-by: Trent Piepho
---
drivers/rtc/rtc-isl1208.c | 37 -
1 file changed, 4 insertions(+), 33 deletions(-)
diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index ec5ef518a09b..37ab3e1d25f5 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -84,29 +84,13 @@ static int
isl1208_i2c_read_regs(struct i2c_client *client, u8 reg, u8 buf[],
unsigned len)
{
- u8 reg_addr[1] = { reg };
- struct i2c_msg msgs[2] = {
- {
- .addr = client->addr,
- .len = sizeof(reg_addr),
- .buf = reg_addr
- },
- {
- .addr = client->addr,
- .flags = I2C_M_RD,
- .len = len,
- .buf = buf
- }
- };
int ret;
WARN_ON(reg > ISL1219_REG_YRT);
WARN_ON(reg + len > ISL1219_REG_YRT + 1);
- ret = i2c_transfer(client->adapter, msgs, 2);
- if (ret > 0)
- ret = 0;
- return ret;
+ ret = i2c_smbus_read_i2c_block_data(client, reg, len, buf);
+ return (ret < 0) ? ret : 0;
}
/* block write */
@@ -114,26 +98,13 @@ static int
isl1208_i2c_set_regs(struct i2c_client *client, u8 reg, u8 const buf[],
unsigned len)
{
- u8 i2c_buf[ISL1208_REG_USR2 + 2];
- struct i2c_msg msgs[1] = {
- {
- .addr = client->addr,
- .len = len + 1,
- .buf = i2c_buf
- }
- };
int ret;
WARN_ON(reg > ISL1219_REG_YRT);
WARN_ON(reg + len > ISL1219_REG_YRT + 1);
- i2c_buf[0] = reg;
- memcpy(_buf[1], [0], len);
-
- ret = i2c_transfer(client->adapter, msgs, 1);
- if (ret > 0)
- ret = 0;
- return ret;
+ ret = i2c_smbus_write_i2c_block_data(client, reg, len, buf);
+ return (ret < 0) ? ret : 0;
}
/* simple check to see whether we have a isl1208 */
--
2.14.4
[PATCH] rtc: isl1208: Use i2c block read/write routines
The Linux i2c layer has functions to execute common SMBUS/I2C
transactions. The register access code here is identical to the I2C
read/write block data routines.
Cc: Alessandro Zummo
Cc: Alexandre Belloni
Signed-off-by: Trent Piepho
---
drivers/rtc/rtc-isl1208.c | 37 -
1 file changed, 4 insertions(+), 33 deletions(-)
diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index ec5ef518a09b..37ab3e1d25f5 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -84,29 +84,13 @@ static int
isl1208_i2c_read_regs(struct i2c_client *client, u8 reg, u8 buf[],
unsigned len)
{
- u8 reg_addr[1] = { reg };
- struct i2c_msg msgs[2] = {
- {
- .addr = client->addr,
- .len = sizeof(reg_addr),
- .buf = reg_addr
- },
- {
- .addr = client->addr,
- .flags = I2C_M_RD,
- .len = len,
- .buf = buf
- }
- };
int ret;
WARN_ON(reg > ISL1219_REG_YRT);
WARN_ON(reg + len > ISL1219_REG_YRT + 1);
- ret = i2c_transfer(client->adapter, msgs, 2);
- if (ret > 0)
- ret = 0;
- return ret;
+ ret = i2c_smbus_read_i2c_block_data(client, reg, len, buf);
+ return (ret < 0) ? ret : 0;
}
/* block write */
@@ -114,26 +98,13 @@ static int
isl1208_i2c_set_regs(struct i2c_client *client, u8 reg, u8 const buf[],
unsigned len)
{
- u8 i2c_buf[ISL1208_REG_USR2 + 2];
- struct i2c_msg msgs[1] = {
- {
- .addr = client->addr,
- .len = len + 1,
- .buf = i2c_buf
- }
- };
int ret;
WARN_ON(reg > ISL1219_REG_YRT);
WARN_ON(reg + len > ISL1219_REG_YRT + 1);
- i2c_buf[0] = reg;
- memcpy(_buf[1], [0], len);
-
- ret = i2c_transfer(client->adapter, msgs, 1);
- if (ret > 0)
- ret = 0;
- return ret;
+ ret = i2c_smbus_write_i2c_block_data(client, reg, len, buf);
+ return (ret < 0) ? ret : 0;
}
/* simple check to see whether we have a isl1208 */
--
2.14.4
Re: [PATCH] PCI: dwc: Limit config space size for i.MX6
On Wed, 2018-11-14 at 16:49 +0100, Stefan Agner wrote: > On 19.10.2018 13:13, Stefan Agner wrote: > > Reading the full 4k config space through sysfs leads to an > > external abort. Testing on a platform showed that the upper > > limit is 512. Limit config space to 512. > > Any comment on this patch? > > Since other devices use similar quirks, I guess the fix can't be far > off? > > Maybe restricting to the PCI device ID used in i.MX 6 only is too > restrictive, but I guess better restrictive for now? To trigger this bug I should read the sysfs "config" file for the PCI bridge device? Tested on imx7, no problems. # hexdump -C /sys/devices/platform/soc/3080.aips-bus/3380.pcie/pci:00/:00:00.0/config [stuff] 0130 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 || * 0400 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff || * 0700 76 00 63 01 ff ff ff ff 04 00 00 07 00 f0 f0 1b |v.c.| [more stuff] The bridge on imx7d is 16c3:abcd, same as patch I believe. I do have a pci-e device connected, unlike the original bug. Maybe that is related? Or maybe this problem is fixed in imx7d? > > > > #define PCI_VENDOR_ID_SYNOPSYS 0x16c3 > > +#define PCI_DEVICE_ID_SYNOPSYS_IMX60xabcd > > > > #define PCI_VENDOR_ID_VITESSE 0x1725 > > #define PCI_DEVICE_ID_VITESSE_VSC7174 0x7174
Re: [PATCH] PCI: dwc: Limit config space size for i.MX6
On Wed, 2018-11-14 at 16:49 +0100, Stefan Agner wrote: > On 19.10.2018 13:13, Stefan Agner wrote: > > Reading the full 4k config space through sysfs leads to an > > external abort. Testing on a platform showed that the upper > > limit is 512. Limit config space to 512. > > Any comment on this patch? > > Since other devices use similar quirks, I guess the fix can't be far > off? > > Maybe restricting to the PCI device ID used in i.MX 6 only is too > restrictive, but I guess better restrictive for now? To trigger this bug I should read the sysfs "config" file for the PCI bridge device? Tested on imx7, no problems. # hexdump -C /sys/devices/platform/soc/3080.aips-bus/3380.pcie/pci:00/:00:00.0/config [stuff] 0130 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 || * 0400 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff || * 0700 76 00 63 01 ff ff ff ff 04 00 00 07 00 f0 f0 1b |v.c.| [more stuff] The bridge on imx7d is 16c3:abcd, same as patch I believe. I do have a pci-e device connected, unlike the original bug. Maybe that is related? Or maybe this problem is fixed in imx7d? > > > > #define PCI_VENDOR_ID_SYNOPSYS 0x16c3 > > +#define PCI_DEVICE_ID_SYNOPSYS_IMX60xabcd > > > > #define PCI_VENDOR_ID_VITESSE 0x1725 > > #define PCI_DEVICE_ID_VITESSE_VSC7174 0x7174
Re: No interrupt was generated using MSI
On Thu, 2018-11-01 at 17:30 -0400, thesve...@gmail.com wrote:
> I am seeing this in my kernel log:
>
> [ 10.235625] tg3 :03:00.0 eth20: No interrupt was generated
> using MSI.
> Switching to INTx mode. Please report this failure to the PCI
> maintainer and include system chipset information
>
> I looked through MAINTAINERS, but I can't find the PCI maintainer.
> Can anyone help?
>
> Chipset info: this happens on a Freescale i.MX6Q. Mainline 4.19
> kernel.
> The devicetree entry for the pcie chipset is here:
>
> arch/arm/boot/dts/imx6qdl.dtsi:
>
> pcie: pcie@1ffc000 {
> compatible = "fsl,imx6q-pcie", "snps,dw-pcie";
>
> };
It sounds like you might be hitting the bug I posted a fix for
recently.
https://patchwork.kernel.org/patch/10657987/
Re: No interrupt was generated using MSI
On Thu, 2018-11-01 at 17:30 -0400, thesve...@gmail.com wrote:
> I am seeing this in my kernel log:
>
> [ 10.235625] tg3 :03:00.0 eth20: No interrupt was generated
> using MSI.
> Switching to INTx mode. Please report this failure to the PCI
> maintainer and include system chipset information
>
> I looked through MAINTAINERS, but I can't find the PCI maintainer.
> Can anyone help?
>
> Chipset info: this happens on a Freescale i.MX6Q. Mainline 4.19
> kernel.
> The devicetree entry for the pcie chipset is here:
>
> arch/arm/boot/dts/imx6qdl.dtsi:
>
> pcie: pcie@1ffc000 {
> compatible = "fsl,imx6q-pcie", "snps,dw-pcie";
>
> };
It sounds like you might be hitting the bug I posted a fix for
recently.
https://patchwork.kernel.org/patch/10657987/
Re: [PATCH] spi: Make GPIO CSs honour the SPI_NO_CS flag
On Tue, 2018-10-16 at 10:03 +0100, Mark Brown wrote: > On Mon, Oct 15, 2018 at 06:34:18PM +0000, Trent Piepho wrote: > > > What about the calls to spi->controller->set_cs() after this? Should a > > driver provided set_cs method be responsible for checking SPI_NO_CS? > > Or should it not be called in the first place? > > This seems like something that should be done entirely in the framework, > no point in every single driver having to open code the same thing. > > > I imagine it depends on what set_cs needs to do, which might not be > > solely related to changing the CS line. > > It should be. If something is hanging other work on set_cs() then it's > going to break. IIRC, for spi-dw setting CS is the only way to trigger the master to do anything. I think orion is the same way. Even if you don't want a CS line the driver still needs to assert one. Which CS to use as the dummy CS is a challenge that has come up before. bcm2835_spi_set_cs() does check SPI_NO_CS, but it still does a lot of other stuff even if that is set, likely because of the above issue.
Re: [PATCH] spi: Make GPIO CSs honour the SPI_NO_CS flag
On Tue, 2018-10-16 at 10:03 +0100, Mark Brown wrote: > On Mon, Oct 15, 2018 at 06:34:18PM +0000, Trent Piepho wrote: > > > What about the calls to spi->controller->set_cs() after this? Should a > > driver provided set_cs method be responsible for checking SPI_NO_CS? > > Or should it not be called in the first place? > > This seems like something that should be done entirely in the framework, > no point in every single driver having to open code the same thing. > > > I imagine it depends on what set_cs needs to do, which might not be > > solely related to changing the CS line. > > It should be. If something is hanging other work on set_cs() then it's > going to break. IIRC, for spi-dw setting CS is the only way to trigger the master to do anything. I think orion is the same way. Even if you don't want a CS line the driver still needs to assert one. Which CS to use as the dummy CS is a challenge that has come up before. bcm2835_spi_set_cs() does check SPI_NO_CS, but it still does a lot of other stuff even if that is set, likely because of the above issue.
Re: [PATCH] spi: Make GPIO CSs honour the SPI_NO_CS flag
On Fri, 2018-10-12 at 10:23 +0100, Phil Elwell wrote:
> The SPI configuration state includes an SPI_NO_CS flag that disables
> all CS line manipulation, for applications that want to manage their
> own chip selects. However, this flag is ignored by the GPIO CS code
> in the SPI framework.
> @@ -729,7 +729,9 @@ static void spi_set_cs(struct spi_device *spi, bool
> enable)
> enable = !enable;
>
> if (gpio_is_valid(spi->cs_gpio)) {
> - gpio_set_value(spi->cs_gpio, !enable);
> + /* Honour the SPI_NO_CS flag */
> + if (!(spi->mode & SPI_NO_CS))
> + gpio_set_value(spi->cs_gpio, !enable);
> /* Some SPI masters need both GPIO CS & slave_select */
> if ((spi->controller->flags & SPI_MASTER_GPIO_SS) &&
> spi->controller->set_cs)
What about the calls to spi->controller->set_cs() after this? Should a
driver provided set_cs method be responsible for checking SPI_NO_CS?
Or should it not be called in the first place?
I imagine it depends on what set_cs needs to do, which might not be
solely related to changing the CS line.
Re: [PATCH] spi: Make GPIO CSs honour the SPI_NO_CS flag
On Fri, 2018-10-12 at 10:23 +0100, Phil Elwell wrote:
> The SPI configuration state includes an SPI_NO_CS flag that disables
> all CS line manipulation, for applications that want to manage their
> own chip selects. However, this flag is ignored by the GPIO CS code
> in the SPI framework.
> @@ -729,7 +729,9 @@ static void spi_set_cs(struct spi_device *spi, bool
> enable)
> enable = !enable;
>
> if (gpio_is_valid(spi->cs_gpio)) {
> - gpio_set_value(spi->cs_gpio, !enable);
> + /* Honour the SPI_NO_CS flag */
> + if (!(spi->mode & SPI_NO_CS))
> + gpio_set_value(spi->cs_gpio, !enable);
> /* Some SPI masters need both GPIO CS & slave_select */
> if ((spi->controller->flags & SPI_MASTER_GPIO_SS) &&
> spi->controller->set_cs)
What about the calls to spi->controller->set_cs() after this? Should a
driver provided set_cs method be responsible for checking SPI_NO_CS?
Or should it not be called in the first place?
I imagine it depends on what set_cs needs to do, which might not be
solely related to changing the CS line.
Re: [PATCH 1/2] dt-bindings: spi: dw: add cs-override property
On Wed, 2018-10-10 at 13:27 +0100, Mark Brown wrote: > > That's great and we get to reuse all the driver code with a quirk (a > quirk which fixes the hardware to be more compatible with devices, this > is a really good hardware change). Ideally we'd be able to enumerate > things like IP versions and options from hardware but that's a more > entertaining problem. Might well be possible here. There are an id code and version registers that could be used. Did anyone have the foresight to change them would modifying the IP is the question. > Having said all this if there are production systems using this > property, especially production systems where people other than the > system integrator can realistically deploy their own kernel separate to > the device tree, then supporting those existing DTs even if they're not > doing the ideal thing might be the best thing. You mentioned that this > might be the case, can you check what the status is there please? I've developed systems that used chips with this SPI master. I used GPIO chip selects to work around the existing behavior and would not be negatively affected if someone fixed this bug. There's also another quirk where certain phase/polarity combination make it generate a CS pulse between each word. IMHO, Linux SPI has a specification for how it works. Setup an xfer a certain way and you get a certain waveform from the master. If the master generates the wrong waveform then it's a bug. Designers should know better than to design a system that depends on a bug never being fixed and be caught off guard when it is. If you want CS to turn off whenever the FIFO is empty (why???) then extend the spec to encompass the behavior you want. Like how "pulse between words" can be specified now.
Re: [PATCH 1/2] dt-bindings: spi: dw: add cs-override property
On Wed, 2018-10-10 at 13:27 +0100, Mark Brown wrote: > > That's great and we get to reuse all the driver code with a quirk (a > quirk which fixes the hardware to be more compatible with devices, this > is a really good hardware change). Ideally we'd be able to enumerate > things like IP versions and options from hardware but that's a more > entertaining problem. Might well be possible here. There are an id code and version registers that could be used. Did anyone have the foresight to change them would modifying the IP is the question. > Having said all this if there are production systems using this > property, especially production systems where people other than the > system integrator can realistically deploy their own kernel separate to > the device tree, then supporting those existing DTs even if they're not > doing the ideal thing might be the best thing. You mentioned that this > might be the case, can you check what the status is there please? I've developed systems that used chips with this SPI master. I used GPIO chip selects to work around the existing behavior and would not be negatively affected if someone fixed this bug. There's also another quirk where certain phase/polarity combination make it generate a CS pulse between each word. IMHO, Linux SPI has a specification for how it works. Setup an xfer a certain way and you get a certain waveform from the master. If the master generates the wrong waveform then it's a bug. Designers should know better than to design a system that depends on a bug never being fixed and be caught off guard when it is. If you want CS to turn off whenever the FIFO is empty (why???) then extend the spec to encompass the behavior you want. Like how "pulse between words" can be specified now.
Re: [PATCH 1/2] dt-bindings: spi: dw: add cs-override property
On Wed, 2018-10-10 at 10:08 +0300, Talel Shenhar wrote: > > The dw spi controller has an auto-deselect of Chip-Select, in case there is > no data inside the Tx FIFO. While working on platforms with Alpine chips, > auto-deselect mode causes an issue for some spi devices that can't handle > the Chip-Select deselect in the middle of a transaction. It is a normal > behavior for a Tx FIFO to be empty in the middle of a transaction, due to > So that's the problem! I, like everyone else I suspect, switched to using GPIO chip selects with this driver because of this. I narrowed it down to a CS de-assert when the bus switched from TX to RX, which of course makes a SPI register read fail on most devices. The TX FIFO would empty at this point, so that would explain it. Did the designers of this IP ever read a SPI device datasheet??? Got to agree with Mark Brown, why would anyone ever want to NOT have it work properly? The previous behavior is not "alternate correct", it's Broken.
Re: [PATCH 1/2] dt-bindings: spi: dw: add cs-override property
On Wed, 2018-10-10 at 10:08 +0300, Talel Shenhar wrote: > > The dw spi controller has an auto-deselect of Chip-Select, in case there is > no data inside the Tx FIFO. While working on platforms with Alpine chips, > auto-deselect mode causes an issue for some spi devices that can't handle > the Chip-Select deselect in the middle of a transaction. It is a normal > behavior for a Tx FIFO to be empty in the middle of a transaction, due to > So that's the problem! I, like everyone else I suspect, switched to using GPIO chip selects with this driver because of this. I narrowed it down to a CS de-assert when the bus switched from TX to RX, which of course makes a SPI register read fail on most devices. The TX FIFO would empty at this point, so that would explain it. Did the designers of this IP ever read a SPI device datasheet??? Got to agree with Mark Brown, why would anyone ever want to NOT have it work properly? The previous behavior is not "alternate correct", it's Broken.
Re: [PATCH 2/2] dw: spi: add support for Alpine spi controller
On Wed, 2018-10-10 at 18:15 +0300, Talel Shenhar wrote: > Add support for a new devicetree compatible string called > 'al,alpine-apb-ssi', which is necessary for the Amazon Alpine spi > controller. 'al,alpine-dw-apb-ssi' is used in the dw spi driver if > specified in the devicetree. Otherwise, fall back to driver default > behavior, i.e. original dw IP hw driver behavior. > > Signed-off-by: Talel Shenhar > Signed-off-by: David Woodhouse Shouldn't this be "amzn,alpine-apb-ssi"? The convention, and this IS documented, is to the use company's stock ticker symbol as the prefix, so as to have something grounded in the real world. I don't know of anyone else using a product line name as the company name in their dt bindings. Example, "snps,dw-apb-ssi", is named for Synopsis, not the Designware line.
Re: [PATCH 2/2] dw: spi: add support for Alpine spi controller
On Wed, 2018-10-10 at 18:15 +0300, Talel Shenhar wrote: > Add support for a new devicetree compatible string called > 'al,alpine-apb-ssi', which is necessary for the Amazon Alpine spi > controller. 'al,alpine-dw-apb-ssi' is used in the dw spi driver if > specified in the devicetree. Otherwise, fall back to driver default > behavior, i.e. original dw IP hw driver behavior. > > Signed-off-by: Talel Shenhar > Signed-off-by: David Woodhouse Shouldn't this be "amzn,alpine-apb-ssi"? The convention, and this IS documented, is to the use company's stock ticker symbol as the prefix, so as to have something grounded in the real world. I don't know of anyone else using a product line name as the company name in their dt bindings. Example, "snps,dw-apb-ssi", is named for Synopsis, not the Designware line.
Re: [PATCH 3/3] eeprom: at25: Split writes in two SPI transfers to optimize DMA
On Wed, 2018-10-10 at 15:40 +0200, Geert Uytterhoeven wrote: > Currently EEPROM writes are implemented using a single SPI transfer, > which contains all of command, address, and payload data bytes. > As some SPI controllers impose limitations on transfers with respect to > the use of DMA, they may have to fall back to PIO. E.g. DMA may require > the transfer length to be a multiple of 4 bytes. > > Optimize writes for DMA by splitting writes in two SPI transfers: > - The first transfer contains command and address bytes, > - The second transfer contains the actual payload data, now stored at > the start of the (kmalloc() aligned) buffer, to improve payload > alignment. Does this always optimize? A master capable of an of aligned 18 byte DMA xfer would now have a 2 byte xfer that would probably be PIO followed by a 16 byte DMA. Or writing 14 bytes to the EEPROM has changed from an aligned 16 byte write to a 2 byte and a 14 byte, which is now worse for the 4 byte multiple requirement master which can use any DMA anymore. It seems like an enhancement to the DMA code to look more like a efficient memcpy() that aligns the address, then xfers efficient blocks, then finishes the sub-block tail would be more generally applicable. Or more simply, given an aligned 18 byte xfer, the driver should do an aligned 16 byte DMA and then two more bytes.
Re: [PATCH 3/3] eeprom: at25: Split writes in two SPI transfers to optimize DMA
On Wed, 2018-10-10 at 15:40 +0200, Geert Uytterhoeven wrote: > Currently EEPROM writes are implemented using a single SPI transfer, > which contains all of command, address, and payload data bytes. > As some SPI controllers impose limitations on transfers with respect to > the use of DMA, they may have to fall back to PIO. E.g. DMA may require > the transfer length to be a multiple of 4 bytes. > > Optimize writes for DMA by splitting writes in two SPI transfers: > - The first transfer contains command and address bytes, > - The second transfer contains the actual payload data, now stored at > the start of the (kmalloc() aligned) buffer, to improve payload > alignment. Does this always optimize? A master capable of an of aligned 18 byte DMA xfer would now have a 2 byte xfer that would probably be PIO followed by a 16 byte DMA. Or writing 14 bytes to the EEPROM has changed from an aligned 16 byte write to a 2 byte and a 14 byte, which is now worse for the 4 byte multiple requirement master which can use any DMA anymore. It seems like an enhancement to the DMA code to look more like a efficient memcpy() that aligns the address, then xfers efficient blocks, then finishes the sub-block tail would be more generally applicable. Or more simply, given an aligned 18 byte xfer, the driver should do an aligned 16 byte DMA and then two more bytes.
Re: [PATCH v2 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation
On Mon, 2018-09-24 at 10:13 -0700, Doug Anderson wrote: > IIUC previous suggestions about just naming it based on the first SoC > was due to the difficulty of coming up with a good generic name to > give something. For instance you definitely wouldn't want to name it > "qcom-qspi-sdm8xx" because you have no idea what future SoCs will be > numbered. And the hypothetical sdm899 might use a non-compatible device that uses a different driver, and that really makes "qcom-qspi-sdm8xx" look dumb. > > In the case here calling it "qcom,qspi-v1" is better than that and if > Rob gives the thumbs up then I won't object to it. In general though > looking at other device tree bindings this doesn't seem like a thing > commonly done. Perhaps if we decide it's useful here we should start > suggesting it everywhere... It would help if the programming model or IP core name or whatever this is using was mentioned in the public reference manual for the SoC. Then is a lot more clear that a number of different SoCs all have the same quad spi controller inside them. Usually it's not like that. The RMs just say, "it's got a SPI master with these registers." What SoCs use the same IP module, which different? When did they make a new version? The silicon vendors don't tell you this. So any name we make up for the IP module likely doesn't match reality.
Re: [PATCH v2 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation
On Mon, 2018-09-24 at 10:13 -0700, Doug Anderson wrote: > IIUC previous suggestions about just naming it based on the first SoC > was due to the difficulty of coming up with a good generic name to > give something. For instance you definitely wouldn't want to name it > "qcom-qspi-sdm8xx" because you have no idea what future SoCs will be > numbered. And the hypothetical sdm899 might use a non-compatible device that uses a different driver, and that really makes "qcom-qspi-sdm8xx" look dumb. > > In the case here calling it "qcom,qspi-v1" is better than that and if > Rob gives the thumbs up then I won't object to it. In general though > looking at other device tree bindings this doesn't seem like a thing > commonly done. Perhaps if we decide it's useful here we should start > suggesting it everywhere... It would help if the programming model or IP core name or whatever this is using was mentioned in the public reference manual for the SoC. Then is a lot more clear that a number of different SoCs all have the same quad spi controller inside them. Usually it's not like that. The RMs just say, "it's got a SPI master with these registers." What SoCs use the same IP module, which different? When did they make a new version? The silicon vendors don't tell you this. So any name we make up for the IP module likely doesn't match reality.
Re: [PATCH v2 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation
On Fri, 2018-09-21 at 10:39 -0700, Mark Brown wrote: > On Fri, Sep 21, 2018 at 10:30:57AM -0700, Stephen Boyd wrote: > > Quoting Ryan Case (2018-09-20 15:40:54) > > > +Required properties: > > > +- compatible: Should contain: > > > + "qcom,sdm845-qspi" > > Does someone have a more generic compatible string that can be added > > here to indicate the type of quad SPI controller this is? I really doubt > > this is a one-off hardware block for the specific SDM845 SoC. > > The idiom for DT is supposed to be to use only device specific names > unfortunately. Basically the "first" device the driver can control has it's specific name used as the generic string. This is used in place of some internal codename for the core. Then a newer device will have "foo,XYZ200", "foo,XYZ100" as compatible, where the 100 was the first device and the 200 is new one. Maybe the driver cares, or will care, about what device this or maybe it can drive the device fine without needing to know more than the generic.
Re: [PATCH v2 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation
On Fri, 2018-09-21 at 10:39 -0700, Mark Brown wrote: > On Fri, Sep 21, 2018 at 10:30:57AM -0700, Stephen Boyd wrote: > > Quoting Ryan Case (2018-09-20 15:40:54) > > > +Required properties: > > > +- compatible: Should contain: > > > + "qcom,sdm845-qspi" > > Does someone have a more generic compatible string that can be added > > here to indicate the type of quad SPI controller this is? I really doubt > > this is a one-off hardware block for the specific SDM845 SoC. > > The idiom for DT is supposed to be to use only device specific names > unfortunately. Basically the "first" device the driver can control has it's specific name used as the generic string. This is used in place of some internal codename for the core. Then a newer device will have "foo,XYZ200", "foo,XYZ100" as compatible, where the 100 was the first device and the 200 is new one. Maybe the driver cares, or will care, about what device this or maybe it can drive the device fine without needing to know more than the generic.
[PATCH] ARM: amba: Fix leak of driver_override attribute value
If driver_override was set when a device was released the string would not be kfree'ed in amba_device_release and thus leaked when the amba device was freed. Cc: Russell King Cc: Todd Kjos Cc: Geert Uytterhoeven Cc: Greg Kroah-Hartman Signed-off-by: Trent Piepho --- drivers/amba/bus.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c index 41b706403ef7..ff3cb96526bc 100644 --- a/drivers/amba/bus.c +++ b/drivers/amba/bus.c @@ -347,6 +347,7 @@ static void amba_device_release(struct device *dev) if (d->res.parent) release_resource(>res); + kfree(d->driver_override); kfree(d); } -- 2.14.4
[PATCH] ARM: amba: Fix leak of driver_override attribute value
If driver_override was set when a device was released the string would not be kfree'ed in amba_device_release and thus leaked when the amba device was freed. Cc: Russell King Cc: Todd Kjos Cc: Geert Uytterhoeven Cc: Greg Kroah-Hartman Signed-off-by: Trent Piepho --- drivers/amba/bus.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c index 41b706403ef7..ff3cb96526bc 100644 --- a/drivers/amba/bus.c +++ b/drivers/amba/bus.c @@ -347,6 +347,7 @@ static void amba_device_release(struct device *dev) if (d->res.parent) release_resource(>res); + kfree(d->driver_override); kfree(d); } -- 2.14.4
Re: [PATCH v2] spi: dw: support 4-16 bits per word
On Fri, 2018-08-17 at 09:01 +0200, Simon Goldschmidt wrote: > The spi-dw driver currently only supports 8 or 16 bits per word. > > Since the hardware supports 4-16 bits per word, adapt the driver > to also support this. > > > @@ -307,15 +307,13 @@ static int dw_spi_transfer_one(struct spi_controller > *master, > + > + if ((transfer->bits_per_word < 4) || (transfer->bits_per_word > 16)) > return -EINVAL; > - } You don't need this check as the spi core validates the transfer against master->bits_per_word_mask. > master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP; > - master->bits_per_word_mask = SPI_BPW_MASK(8) | SPI_BPW_MASK(16); > + master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 16); > master->bus_num = dws->bus_num; >
Re: [PATCH v2] spi: dw: support 4-16 bits per word
On Fri, 2018-08-17 at 09:01 +0200, Simon Goldschmidt wrote: > The spi-dw driver currently only supports 8 or 16 bits per word. > > Since the hardware supports 4-16 bits per word, adapt the driver > to also support this. > > > @@ -307,15 +307,13 @@ static int dw_spi_transfer_one(struct spi_controller > *master, > + > + if ((transfer->bits_per_word < 4) || (transfer->bits_per_word > 16)) > return -EINVAL; > - } You don't need this check as the spi core validates the transfer against master->bits_per_word_mask. > master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP; > - master->bits_per_word_mask = SPI_BPW_MASK(8) | SPI_BPW_MASK(16); > + master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 16); > master->bus_num = dws->bus_num; >
Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
On Thu, 2018-08-09 at 12:37 -0700, Doug Anderson wrote: > On Thu, Aug 9, 2018 at 11:24 AM, Trent Piepho wrote: > > > I think we're in agreement but perhaps there's a miscommunication here? > > I'm saying that we _shouldn't_ put the max-speed of the master in the > device tree. The max speed for the IP block ought to be in code > either in the clock driver or in the SPI driver based on the > compatible string. Yes, totally agree. Usually the clock framework provides a clock to the SPI IP block at some rate, or range of rates. Based on that clock, the SPI IP block has a max SPI clock, and the driver can calculate it. We do have a concept of a SPI master max clock, but it's never needed to be specified in the DT. Consider userspace access via spidev. The transfer asks for 100 MHz, but the spi master driver can not possibly program that speed into whatever register(s) control the spi clock. The SPI core handles that case of limiting a transfer to the master's max. > ...as one argument _against_ putting a max-speed for the master in the > device tree I'm saying that we can already specify a max-speed of each > slave in the device tree. The max speed of the slave should take into > account whatever factors are specific to this board including trace > lengths, noise, etc. If we somehow did need to get a max speed in the > device tree it seems like we could just adjust the max speed for the > slave to be something lower. In other words if you know a board has > an sdm845 on it and you know that the SPI clock on sdm845 can't go > over 50 MHz then it would make no sense to suggest that we should run > the SPI clock for a device at 100 MHz. What you might do is determine the slave can run at 100 MHz, in some cases. The board, traces, master, etc. can do it, in some cases. But it's also possible to change the clocking design or settings at a higher level, which trickles down to the SPI master IP block having a lower clock. This is handled by the master max speed being less then the slave's max speed. The master max wouldn't be in the DT, since it's probably determined at run time based on input clock to the master. Based on the SoC core clock, voltage, power mode, etc. So you might have a DT with a slave at 100 MHz, even if in some cases the master's max is less than 100 MHz. It's ok to depend on the master to be the limiting factor.
Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
On Thu, 2018-08-09 at 12:37 -0700, Doug Anderson wrote: > On Thu, Aug 9, 2018 at 11:24 AM, Trent Piepho wrote: > > > I think we're in agreement but perhaps there's a miscommunication here? > > I'm saying that we _shouldn't_ put the max-speed of the master in the > device tree. The max speed for the IP block ought to be in code > either in the clock driver or in the SPI driver based on the > compatible string. Yes, totally agree. Usually the clock framework provides a clock to the SPI IP block at some rate, or range of rates. Based on that clock, the SPI IP block has a max SPI clock, and the driver can calculate it. We do have a concept of a SPI master max clock, but it's never needed to be specified in the DT. Consider userspace access via spidev. The transfer asks for 100 MHz, but the spi master driver can not possibly program that speed into whatever register(s) control the spi clock. The SPI core handles that case of limiting a transfer to the master's max. > ...as one argument _against_ putting a max-speed for the master in the > device tree I'm saying that we can already specify a max-speed of each > slave in the device tree. The max speed of the slave should take into > account whatever factors are specific to this board including trace > lengths, noise, etc. If we somehow did need to get a max speed in the > device tree it seems like we could just adjust the max speed for the > slave to be something lower. In other words if you know a board has > an sdm845 on it and you know that the SPI clock on sdm845 can't go > over 50 MHz then it would make no sense to suggest that we should run > the SPI clock for a device at 100 MHz. What you might do is determine the slave can run at 100 MHz, in some cases. The board, traces, master, etc. can do it, in some cases. But it's also possible to change the clocking design or settings at a higher level, which trickles down to the SPI master IP block having a lower clock. This is handled by the master max speed being less then the slave's max speed. The master max wouldn't be in the DT, since it's probably determined at run time based on input clock to the master. Based on the SoC core clock, voltage, power mode, etc. So you might have a DT with a slave at 100 MHz, even if in some cases the master's max is less than 100 MHz. It's ok to depend on the master to be the limiting factor.
Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
On Fri, 2018-08-10 at 21:59 +0530, dk...@codeaurora.org wrote: > > Here are my couple of cents: > SPI controller maximum frequency can be lesser than or equal to Clock > framework's maximum > frequency, so should not rely on the Clock framework. But there is probably some means, via the controller's IP block input clock, combined with the controller's IP version (from the compatible string and a device data table), to derive the controller's max clock. At least, AFAICT, this has been the case for all existing master drivers. > > Now the need is, how to communicate the SPI controller maximum frequency > to SPI core framework? > Is it by DTSI entry or hardcoding in the SPI controller driver? > > My stand is for providing the DTSI entry. > Why because, this keeps SPI controller driver generic across the boards > and portable. > Also it is not against to Device tree usage because maximum frequency > is describing the property of the hardware. My point is that not everything that possibly can go in the device tree should be placed there, just because it can be. If the master driver can figure out its max speed, let it do that.
Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
On Fri, 2018-08-10 at 21:59 +0530, dk...@codeaurora.org wrote: > > Here are my couple of cents: > SPI controller maximum frequency can be lesser than or equal to Clock > framework's maximum > frequency, so should not rely on the Clock framework. But there is probably some means, via the controller's IP block input clock, combined with the controller's IP version (from the compatible string and a device data table), to derive the controller's max clock. At least, AFAICT, this has been the case for all existing master drivers. > > Now the need is, how to communicate the SPI controller maximum frequency > to SPI core framework? > Is it by DTSI entry or hardcoding in the SPI controller driver? > > My stand is for providing the DTSI entry. > Why because, this keeps SPI controller driver generic across the boards > and portable. > Also it is not against to Device tree usage because maximum frequency > is describing the property of the hardware. My point is that not everything that possibly can go in the device tree should be placed there, just because it can be. If the master driver can figure out its max speed, let it do that.
Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
On Thu, 2018-08-09 at 11:03 -0700, Doug Anderson wrote:
> On Fri, Aug 3, 2018 at 5:18 AM, wrote:
> > > > + if (of_property_read_u32(pdev->dev.of_node, "spi-max-frequency",
> > > > + >max_speed_hz)) {
> >
> >
> > > Why does this need to come from DT?
> >
> >
> > This is required to set the SPI controller max frequency.
> > As it is specific to the controller, so looks meaningful to specify it in
> > dtsi.
> > Also, spi core framework will set the transfer speed to controller max
> > frequency
> > if transfer frequency is greater than controller max frequency.
> > Please mention if you have a other opinion.
>
> Here are my thoughts:
>
> 1. It sure seems like the clock framework could be enforcing the max
> speed here. SPI can just ask for the speed and the clock framework
> will pick the highest speed it can if you ask for one too high. Isn't
> that the whole point of the "struct freq_tbl" in the clock driver?
>
>
> 2. The device tree writer already provides a max clock speed for each
> SPI slave in the device tree. ...shouldn't the device tree writer
> already be taking into account the max of the SPI port when setting
> this value?
No, the way it works is the actual speed is the lesser of the master's
max and the slave device's max.
But usually the master's max is determined by:
1. The input clock the SPI master device, as provided by the clock
framework. Usually the max SPI clock will be this clock /2 or
something like that.
2. The master has some maximum clock as part of its specifications, in
which case the driver basically hard codes it. Maybe it is different
based on model and the driver has a table.
The device tree isn't really meant to be a way to remove all data from
the device driver just because it can be, or shift work from the driver
author to the device tree author.
So, one shouldn't specify the master max in the DT if the driver could
figure it out.
Sometimes you see a field in the DT and I think the thought process
that put it there was, "I don't understand how to set this register,
I'll just stick in the device tree and then it's Someone Else's Problem
to find the correct value."
The max speed that some board can talk to a SPI slave might be from the
specs of the slave device or maybe it's due to the traces on the board
itself that is the limiting factor. In the latter case the convention
is to consider this part of the slave's max speed and put into the DT
in the slave's DT node max speed property.
So the same spi eeprom chip might have have a max in the DT of 20 MHz
on one board, copied out of the eeprom's datasheet. But on another
board it has a max of 10 MHz because on that board's layout it only
works up to 10.
Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
On Thu, 2018-08-09 at 11:03 -0700, Doug Anderson wrote:
> On Fri, Aug 3, 2018 at 5:18 AM, wrote:
> > > > + if (of_property_read_u32(pdev->dev.of_node, "spi-max-frequency",
> > > > + >max_speed_hz)) {
> >
> >
> > > Why does this need to come from DT?
> >
> >
> > This is required to set the SPI controller max frequency.
> > As it is specific to the controller, so looks meaningful to specify it in
> > dtsi.
> > Also, spi core framework will set the transfer speed to controller max
> > frequency
> > if transfer frequency is greater than controller max frequency.
> > Please mention if you have a other opinion.
>
> Here are my thoughts:
>
> 1. It sure seems like the clock framework could be enforcing the max
> speed here. SPI can just ask for the speed and the clock framework
> will pick the highest speed it can if you ask for one too high. Isn't
> that the whole point of the "struct freq_tbl" in the clock driver?
>
>
> 2. The device tree writer already provides a max clock speed for each
> SPI slave in the device tree. ...shouldn't the device tree writer
> already be taking into account the max of the SPI port when setting
> this value?
No, the way it works is the actual speed is the lesser of the master's
max and the slave device's max.
But usually the master's max is determined by:
1. The input clock the SPI master device, as provided by the clock
framework. Usually the max SPI clock will be this clock /2 or
something like that.
2. The master has some maximum clock as part of its specifications, in
which case the driver basically hard codes it. Maybe it is different
based on model and the driver has a table.
The device tree isn't really meant to be a way to remove all data from
the device driver just because it can be, or shift work from the driver
author to the device tree author.
So, one shouldn't specify the master max in the DT if the driver could
figure it out.
Sometimes you see a field in the DT and I think the thought process
that put it there was, "I don't understand how to set this register,
I'll just stick in the device tree and then it's Someone Else's Problem
to find the correct value."
The max speed that some board can talk to a SPI slave might be from the
specs of the slave device or maybe it's due to the traces on the board
itself that is the limiting factor. In the latter case the convention
is to consider this part of the slave's max speed and put into the DT
in the slave's DT node max speed property.
So the same spi eeprom chip might have have a max in the DT of 20 MHz
on one board, copied out of the eeprom's datasheet. But on another
board it has a max of 10 MHz because on that board's layout it only
works up to 10.
Re: [PATCH 2/2] spi: sprd: Add SPI driver for Spreadtrum SC9860
On Wed, 2018-08-08 at 11:19 +0800, Baolin Wang wrote:
> On 8 August 2018 at 01:10, Trent Piepho wrote:
> > On Tue, 2018-08-07 at 18:43 +0800, Baolin Wang wrote:
> > >
> > > +static u32 sprd_spi_transfer_max_timeout(struct sprd_spi *ss,
> > > + struct spi_transfer *t)
> > > +{
> > > + /*
> > > + * The time spent on transmission of the full FIFO data is the
> > > maximum
> > > + * SPI transmission time.
> > > + */
> > > + u32 size = t->bits_per_word * SPRD_SPI_FIFO_SIZE;
> > > + u32 bit_time_us = SPRD_SPI_HZ / t->speed_hz + 1;
There's another flaw here in that the transfer speed, t->speed_hz,
might not be exactly what is used, due to limitations of the clock
divider. It would be better to find the actual SPI clock used, then
use that in the calculations.
> > > + u32 total_time_us = size * bit_time_us;
> > > + /*
> > > + * There is an interval between data and the data in our SPI
> > > hardware,
> > > + * so the total transmission time need add the interval time.
> > > + *
> > > + * The inteval calculation formula:
> > > + * interval time (source clock cycles) = interval * 4 + 10.
> > > + */
> > > + u32 interval_cycle = SPRD_SPI_FIFO_SIZE * ((ss->interval << 2) +
> > > 10);
> > > + u32 interval_time_us = interval_cycle * SPRD_SPI_HZ / ss->src_clk +
> > > 1;
> >
> > If interval is greater than 31, this will overflow.
>
> Usually we will not set such a big value for interval, but this is a
> risk like you said. So we will check and limit the interval value to
> make sure the formula will not overflow.
>
Better would be to limit the inter word delay to whatever maximum value
your hardware supports, and then write code that can calculate that
without error.
> > > +static void sprd_spi_set_speed(struct sprd_spi *ss, u32 speed_hz)
> > > +{
> > > + /*
> > > + * From SPI datasheet, the prescale calculation formula:
> > > + * prescale = SPI source clock / (2 * SPI_freq) - 1;
> > > + */
> > > + u32 clk_div = ss->src_clk / (speed_hz << 1) - 1;
> >
> > You should probably round up here. The convention is to use the
> > closest speed less than equal to the requested speed, but since this is
> > a divider, rounding it down will select the next highest speed.
>
> Per the datasheet, we do not need round up/down the speed. Since our
> hardware can handle the clock calculated by the above formula if the
> requested speed is in the normal region (less than ->max_speed_hz).
That is not what I mean. Let me explain differently.
An integer divider like this can not produce any frequency exactly.
Consider if src_clk is 10 MHz. A clk_div value of 0 produces a 5 MHz
SPI clock. A clk_div value of 1 produces a 2.5 MHz SPI clock.
What if the transfer requests a SPI clock of 3 MHz?
Your math above will produce a SPI clock of 5 MHz, faster than
requested. This is not the convention in Linux SPI masters. You
should instead of have chosen a clk_div value of 1 to get a SPI clock
of 2.5 MHz, the closest clock possible that is not greater than the
requested value.
To do this, round up.
clk_div = DIV_ROUND_UP(ss->src_clk, speed_hz * 2) - 1;
>
Re: [PATCH 2/2] spi: sprd: Add SPI driver for Spreadtrum SC9860
On Wed, 2018-08-08 at 11:19 +0800, Baolin Wang wrote:
> On 8 August 2018 at 01:10, Trent Piepho wrote:
> > On Tue, 2018-08-07 at 18:43 +0800, Baolin Wang wrote:
> > >
> > > +static u32 sprd_spi_transfer_max_timeout(struct sprd_spi *ss,
> > > + struct spi_transfer *t)
> > > +{
> > > + /*
> > > + * The time spent on transmission of the full FIFO data is the
> > > maximum
> > > + * SPI transmission time.
> > > + */
> > > + u32 size = t->bits_per_word * SPRD_SPI_FIFO_SIZE;
> > > + u32 bit_time_us = SPRD_SPI_HZ / t->speed_hz + 1;
There's another flaw here in that the transfer speed, t->speed_hz,
might not be exactly what is used, due to limitations of the clock
divider. It would be better to find the actual SPI clock used, then
use that in the calculations.
> > > + u32 total_time_us = size * bit_time_us;
> > > + /*
> > > + * There is an interval between data and the data in our SPI
> > > hardware,
> > > + * so the total transmission time need add the interval time.
> > > + *
> > > + * The inteval calculation formula:
> > > + * interval time (source clock cycles) = interval * 4 + 10.
> > > + */
> > > + u32 interval_cycle = SPRD_SPI_FIFO_SIZE * ((ss->interval << 2) +
> > > 10);
> > > + u32 interval_time_us = interval_cycle * SPRD_SPI_HZ / ss->src_clk +
> > > 1;
> >
> > If interval is greater than 31, this will overflow.
>
> Usually we will not set such a big value for interval, but this is a
> risk like you said. So we will check and limit the interval value to
> make sure the formula will not overflow.
>
Better would be to limit the inter word delay to whatever maximum value
your hardware supports, and then write code that can calculate that
without error.
> > > +static void sprd_spi_set_speed(struct sprd_spi *ss, u32 speed_hz)
> > > +{
> > > + /*
> > > + * From SPI datasheet, the prescale calculation formula:
> > > + * prescale = SPI source clock / (2 * SPI_freq) - 1;
> > > + */
> > > + u32 clk_div = ss->src_clk / (speed_hz << 1) - 1;
> >
> > You should probably round up here. The convention is to use the
> > closest speed less than equal to the requested speed, but since this is
> > a divider, rounding it down will select the next highest speed.
>
> Per the datasheet, we do not need round up/down the speed. Since our
> hardware can handle the clock calculated by the above formula if the
> requested speed is in the normal region (less than ->max_speed_hz).
That is not what I mean. Let me explain differently.
An integer divider like this can not produce any frequency exactly.
Consider if src_clk is 10 MHz. A clk_div value of 0 produces a 5 MHz
SPI clock. A clk_div value of 1 produces a 2.5 MHz SPI clock.
What if the transfer requests a SPI clock of 3 MHz?
Your math above will produce a SPI clock of 5 MHz, faster than
requested. This is not the convention in Linux SPI masters. You
should instead of have chosen a clk_div value of 1 to get a SPI clock
of 2.5 MHz, the closest clock possible that is not greater than the
requested value.
To do this, round up.
clk_div = DIV_ROUND_UP(ss->src_clk, speed_hz * 2) - 1;
>
