-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable I2C#6 instead of I2C#8 #12
Enable I2C#6 instead of I2C#8 #12
Conversation
Almost there, I would leave few comments, feel free to address them and resend this PR. P.S. Looks pretty much cool! Thanks for your work! |
} | ||
} | ||
|
||
int mrfld_pinconfig_protected(unsigned int pin, u32 mask, u32 bits) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is an API it has to get a pin control device first, something like
ret = syscon_get_by_driver_data(X86_SYSCON_SCU, &dev);
The idea behind is to get lazy probe (that's called "Driver Model" in U-Boot).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andy-shev
Thanks. Makes sense.
But it seems like probe is already automagically called just by the sheer addition of the probe callback, doesn't it? I'm pretty sure I'm missing something, of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you call function before device is probed, it wouldn't work and crash everything. ->probe() will be called when we try to get pointer to a device.
arch/x86/cpu/tangier/pinmux.c
Outdated
|
||
static int tangier_pinctrl_probe(struct udevice *dev) | ||
{ | ||
void *base_addr = syscon_get_first_range(X86_SYSCON_PINCONF); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind indentation.
arch/x86/cpu/tangier/pinmux.c
Outdated
|
||
int mrfld_pinconfig_protected(unsigned int pin, u32 mask, u32 bits) | ||
{ | ||
struct mrfld_family *mf = &i2c_family; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to keep your families somewhere else and here convert pin to family followed by family to bufcfg address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Something like mrfld_get_family
from pinctrl-merrifield.c?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
arch/x86/cpu/tangier/pinmux.c
Outdated
} | ||
|
||
static const struct udevice_id tangier_pinctrl_match[] = { | ||
{ .compatible = "intel,pinctrl-tangier", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One line?
I mean compatible string with second parameter can be on one line. can they?
arch/x86/cpu/tangier/pinmux.c
Outdated
|
||
ret = scu_ipc_raw_command(cmd, 0, &v, 4, NULL, 0, (u32) bufcfg, 0); | ||
if (ret) | ||
printf("Failed to set bufcfg via SCU IPC for pin %u (%d)\n", pin, ret); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I don't remember if err()
stops an execution, please check and use it instead.
arch/x86/cpu/tangier/pinmux.c
Outdated
|
||
v = (value & ~mask) | (bits & mask); | ||
|
||
debug("scu: cmd: %d v: 0x%x p: 0x%x bits: %d, mask: %d bufcfg: 0x%p\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt it's useful in production. (Note ahead, I'm wondering if upstream will ask to use regmap for this driver, which, I guess, has a facility to dump registers)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not useful but this was extremely useful for me to get the parameters right. I'll get rid of it as soon as we get everything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine to me as well, but should be removed before sending upstream.
@@ -0,0 +1,100 @@ | |||
// SPDX-License-Identifier: GPL-2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, like I said in the bug report, check pinctrl-sandbox.c for a hint how this should look like. (Yes, I understand that we are creating a limited driver, it needs to be taken into account)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. So we're making it the UCLASS_PINCTRL, then? With some of pinctrl_ops implemented? And then we'll configure the pins in edison.c using the pinctrl API? Is this the plan? If so, I'll need some time adapting this code to the pinctrl API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure that this what upstream ask us during review. However, we can try first with reduced solution. I guess, we may send it upstream and see what they will tell. In any case it would be nice to have your stuff in my tree sooner.
4ee1a06
to
d9f450c
Compare
This interface will be used to configure properly some pins on Merrifield that are shared with SCU. scu_ipc_raw_command() writes SPTR and DPTR registers before sending a command to SCU. This code has been ported from Linux work done by Andy Shevchenko. Signed-off-by: Georgii Staroselskii <georgii.staroselskii@emlid.com>
d9f450c
to
4936fa1
Compare
@andy-shev I addressed most of your concerns and made a force push. I also fixed most of the warnings checkpatch.pl gave along the way. |
arch/x86/cpu/tangier/pinmux.c
Outdated
unsigned int bufno; | ||
|
||
family = mrfld_get_family(pinctrl, pin); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant blank line
arch/x86/cpu/tangier/pinmux.c
Outdated
}; | ||
|
||
static const struct mrfld_family *mrfld_get_family(struct mrfld_pinctrl *mp, | ||
unsigned int pin) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dunno if it's GH web or in your sources you have this indentation issue (on the second line u
should be in the same column as s
on the line above)
Another possibility is to split like
static const struct mrfld_family *
mrfld_get_fmaily(...)
arch/x86/cpu/tangier/pinmux.c
Outdated
} | ||
|
||
static void __iomem *mrfld_get_bufcfg(struct mrfld_pinctrl *pinctrl, | ||
unsigned int pin) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same about indentation
arch/x86/cpu/tangier/pinmux.c
Outdated
} | ||
|
||
static void mrfld_setup_families(void *base_addr, struct mrfld_family *families, | ||
unsigned int nfam) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with indentation.
arch/x86/cpu/tangier/pinmux.c
Outdated
v = (value & ~mask) | (bits & mask); | ||
|
||
debug("scu: v: 0x%x p: 0x%x bits: %d, mask: %d bufcfg: 0x%p\n", | ||
v, (u32)bufcfg, bits, mask, bufcfg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation.
arch/x86/cpu/tangier/pinmux.c
Outdated
v, (u32)bufcfg, bits, mask, bufcfg); | ||
|
||
ret = scu_ipc_raw_command(IPCMSG_INDIRECT_WRITE, 0, &v, 4, | ||
NULL, 0, (u32)bufcfg, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
arch/x86/cpu/tangier/pinmux.c
Outdated
struct mrfld_pinctrl *pinctrl = dev_get_priv(dev); | ||
|
||
mrfld_setup_families(base_addr, mrfld_families, | ||
ARRAY_SIZE(mrfld_families)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One line?
Please, address some minor issues and I'm ready to take it. |
This API is going to be used to configure some pins that are protected for simple modification. It's not a comprehensive pinctrl driver but can be turned into one when we need this in the future. Now it is planned to be used only in one place. So that's why I decided not to polute the codebase with a full-blown pinctrl-merrifield nobody will use. The code has been ported from Linux work done by Andy Shevchenko in pinctrl-merrfifield.c Signed-off-by: Georgii Staroselskii <georgii.staroselskii@emlid.com>
Initial configuration came from the firmware. Which quite likely has been used in the phones, where I2C #8, that is not part of Atom peripheral, is in use. Thus we need to override the leftover. In order to achieve this goal we make use of the new code in pinmux.c and set the appropriate pin function. From now on we can use I2C#6 in Linux kernel. Signed-off-by: Georgii Staroselskii <georgii.staroselskii@emlid.com>
Now that we have I2C#6 working, it's time to add a ACPI binding. Signed-off-by: Georgii Staroselskii <georgii.staroselskii@emlid.com>
4936fa1
to
869c471
Compare
Hi, I think I got it this time. I set ts=8 sw=8 and see that I have scu_ipc_raw_command() properly aligned in Vim but GH shows them a little unaligned. Is this an issue? |
Compiling U-Boot with ubsan/asan libraries and running it in sandbox may lead to below backtrace: => avb init 0 => avb verify ## Android Verified Boot 2.0 version 1.1.0 read_is_device_unlocked not supported yet common/avb_verify.c:407:31: runtime error: division by zero AddressSanitizer:DEADLYSIGNAL Reviewed-by: Igor Opaniuk <igor.opaniuk@linaro.org> ================================================================= ==9388==ERROR: AddressSanitizer: FPE on unknown address 0x0000004b467f \ (pc 0x0000004b467f bp 0x000000000000 sp 0x7ffd899fe150 T0) #0 0x4b467e in mmc_byte_io common/avb_verify.c:407 #1 0x4b4c47 in mmc_byte_io common/avb_verify.c:532 #2 0x4b4c47 in read_from_partition common/avb_verify.c:533 #3 0x69dc0d in load_and_verify_vbmeta lib/libavb/avb_slot_verify.c:560 andy-shev#4 0x6a1ee6 in avb_slot_verify lib/libavb/avb_slot_verify.c:1139 #5 0x45dabd in do_avb_verify_part cmd/avb.c:245 #6 0x4af77c in cmd_call common/command.c:499 andy-shev#7 0x4af77c in cmd_process common/command.c:538 #8 0x46bafc in run_pipe_real common/cli_hush.c:1677 #9 0x46bafc in run_list_real common/cli_hush.c:1875 #10 0x46c780 in run_list common/cli_hush.c:2024 #11 0x46c780 in parse_stream_outer common/cli_hush.c:3216 andy-shev#12 0x46d34b in parse_file_outer common/cli_hush.c:3299 andy-shev#13 0x4ad609 in cli_loop common/cli.c:217 andy-shev#14 0x4625ae in main_loop common/main.c:65 u-boot#15 0x46f2d1 in run_main_loop common/board_r.c:648 u-boot#16 0x640253 in initcall_run_list lib/initcall.c:30 u-boot#17 0x46f9d0 in board_init_r common/board_r.c:879 u-boot#18 0x40539b in main arch/sandbox/cpu/start.c:321 u-boot#19 0x7fa94925f82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f) u-boot#20 0x408908 in _start (/srv/R/u-boot-master/u-boot+0x408908) AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: FPE common/avb_verify.c:407 in mmc_byte_io ==9388==ABORTING Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
On aarch64 running with dcache off, will result in an unaligned access exception: => dcache off => hash sha1 $kernel_addr_r 100 "Synchronous Abort" handler, esr 0x96000061 elr: 00000000960317d8 lr : 00000000960316a4 (reloc) elr: 00000000fbd787d8 lr : 00000000fbd786a4 [..] The compiler emits a "stur x1, [x0, #12]". x1 is might just be 32 bit aligned pointer. Remove the unused u64 element from the union to drop the minimal alignment to 32 bit. Also remove the union, because it is no more needed. Signed-off-by: Michael Walle <michael@walle.cc> Reviewed-by: Priyanka Jain <priyanka.jain@nxp.com>
Initial I2C#6 pins' configuration came from the firmware. Which quite likely has been used in the phones, where I2C#8, that is not part of Atom peripheral, is in use. Thus we need to override the leftover.
In order to achieve that I needed to port some of your Linux kernel code to U-Boot: