Skip to content
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

[DNM] soc: arm: cypress: psoc6: Introduce pinctrl #28645

Closed

Conversation

nandojve
Copy link
Member

@nandojve nandojve commented Sep 23, 2020

Introduce pinctrl capability on Cypress PSoC-6 SoC. This add SoC support to configure port pins with flags using device tree.
Add myself as codeowner since PSoC-6 is orphan

This is a split from #28527.
It can be merged only after #28779.

Signed-off-by: Gerson Fernando Budke [email protected]

@nandojve nandojve added this to the v2.5.0 milestone Sep 23, 2020
@github-actions github-actions bot added area: Devicetree area: Devicetree Binding PR modifies or adds a Device Tree binding labels Sep 23, 2020
@nandojve nandojve mentioned this pull request Sep 23, 2020
@nandojve nandojve force-pushed the atl/psoc6-zephyr-pinctrl branch 2 times, most recently from 1f0c490 to 6fb8686 Compare September 24, 2020 02:32
@github-actions github-actions bot added the area: API Changes to public APIs label Sep 24, 2020
@nandojve nandojve requested a review from galak September 24, 2020 18:02
@galak galak requested a review from erwango September 24, 2020 18:06
@galak
Copy link
Collaborator

galak commented Sep 24, 2020

@erwango FYI on dts/bindings/pinctrl/pincfg-node.yaml in this PR. We might want to pull that out into its own PR so it can be used for the ST pinctrl as well.

nandojve and others added 4 commits September 25, 2020 18:01
To elaborate more advanced macros for dts related bindings the
<sys/util.h> can not be include.  It create garbage at
<board>.dts.pre.tmp file and have path include issues.  This
moves all macros from util.h to util_macro.h  This allows that
<sys/util_macro.h> can be include on files like
<dts/arm/<manufacturer>/pinctrl_<manufacturer>_<soc>.h.

The issue was raised when try create a macro for pincrtl with
a flag list with variable length.

Signed-off-by: Gerson Fernando Budke <[email protected]>
Many data items that are represented in a pin configuration node are
common and generic. Pin control bindings should use the properties
defined below where they are applicable; not all of these properties
are relevant or useful for all hardware or binding structures. Each
individual binding document should state which of these generic
properties, if any, are used, and the structure of the DT nodes that
contain these properties.

This is based on Linux, documentation:
https://www.kernel.org/doc/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml

Add pincfg-node.yaml properties and refactor atmel,sam-pinctrl.yaml
to use these properties.

Signed-off-by: Gerson Fernando Budke <[email protected]>
Introduce pinctrl capability on Cypress PSoC-6 SoC.  This add SoC
support to configure port pins with flags using device tree.

Signed-off-by: Gerson Fernando Budke <[email protected]>
PSoC-6 SoC needs that user define the nvic interrupt number
to bind with the peripheral interrupt line for the Cortex-M0+
CPU.  It uses a multiplex before any NVIC interrupt line.
The minimal support was added to allow interrupts be available
for both CPUs.

Note: The PSoC-6 SoC allows that both CPUs receive the same
interrupt.  A tipical use is GPIO interrupt handle and user is
responsable to define interrupt line, priority and take care
of enable same peripheral instance on both CPUs only when
appropriated.

Signed-off-by: Gerson Fernando Budke <[email protected]>
@erwango
Copy link
Member

erwango commented Sep 28, 2020

@erwango FYI on dts/bindings/pinctrl/pincfg-node.yaml in this PR. We might want to pull that out into its own PR so it can be used for the ST pinctrl as well.

I agree reusing the same binding makes sense, and I don't see any reason why it shouldn't work. Though, a mentioned:

  Each individual binding document
  should state which of these generic properties, if any, are used, and the
  structure of the DT nodes that contain these properties.

So:

  • we need to overwrite each of the property used anyway
  • this makes a number of non supported properties available to the user (which would compile but get no effect), while today we get an error when a non supported property is set:
devicetree error: 'skew-delay' appears in /soc/pin-controller@48000000/usart1_tx_pb6 in disco_l475_iot1.dts.pre.tmp, but is not declared in 'properties:' in /local/mcu/zephyrproject/zephyr/dts/bindings/pinctrl/st,stm32-pinctrl.yaml

Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you pull the include: sys: util: Move macros from util to util_macro into its own PR so the guys that maintain that can review and comment.

Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should 'soc: arm: cypress: psoc6: Add Cortex-M0+ mux support' be part of the PR?

Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some initial comments.

* NODE = p<port>_<pin>_<inst>_<signal>
*
* NODE: NODE {
* cypress,pins = < &p<port> <port> <pin> HSIOM_SEL_<hsiom> >;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if pin information can be encoded into an uint32, similar to what we do on STM32: https:/zephyrproject-rtos/zephyr/blob/master/include/dt-bindings/pinctrl/stm32-pinctrl.h#L42

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it already is at end:

This is the structure that will handle:

struct soc_gpio_pin {
	GPIO_PRT_Type *regs; /** pointer to registers of the GPIO controller */
	uint32_t pinum;      /** pin number */
	uint32_t flags;      /** pin flags/attributes */
};

The soc/arm/cypress/common/cypress_psoc6_dt.h have a macro CY_PSOC6_DT_PIN that will collect from DT and create above structure at driver level at build time.

The DT_CYPRESS_HSIOM is a variable length argument list that accept flags:
First 5 parameters are mandatory, than flags are added one by one with commas.
DT_CYPRESS_HSIOM(uart5, rx, 5, 0, act_6);
DT_CYPRESS_HSIOM(uart5, rx, 5, 0, act_6, bias-pull-up);
DT_CYPRESS_HSIOM(uart5, rx, 5, 0, act_6, bias-pull-up, input-enable [, etc]);

Above all entries are valid and create a node like:

 p5_0_uart5_rx: p5_0_uart5_rx {
    cypress,pins = <&p5 0x5 0x0 0x12 >;
    bias-pull-up;
    input-enable;
}

The CY_PSOC6_DT_PIN will combine 0x12 (function) + bias-pull-up + input-enable as flags. From &p5 we get reg addr and 0x0 is the pin itself at p5. The remaining 0x5 is the uart instance number and doesn't have nothing to do with pinctrl and probably will be dropped. It was necessary to solve the chicken and egg problem when platform doesn't have nothing.

DT_CYPRESS_HSIOM(uart5, rx, 5, 0, act_6, input-enable);
DT_CYPRESS_HSIOM(uart5, tx, 5, 1, act_6, drive-push-pull);
DT_CYPRESS_HSIOM(uart6, rx, 13, 0, act_6, input-enable);
DT_CYPRESS_HSIOM(uart6, tx, 13, 1, act_6, drive-push-pull);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with this SoC, but if the number of pinctrl options is large, you may consider making pinctrl part of the SoC HAL as we've done for ST.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have intention to move this to HAL and mix Zephyr with Cypress.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't have to be in PDL directly. It could be inside a dts/ at cypress module root.

cypress,pin-cells:
- port
- pin
- hsiom
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this commit introduce gpio bindings? Shouldn't it be pinctrl stuff only? I think it's better to split gpio and pinctrl stuff, they are in principle orthogonal things.

Copy link
Member Author

@nandojve nandojve Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not right, To solve IRQ, GPIO, PINCTRL and noted that the right sequence is:

dt-bindings/dt-util.h: #28779
IRQ: #29459
GPIO: #29489
PINCTRL: This PR

* This is the linker script for both standard images and XIP images.
*/

#include <autoconf.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change should likely go into a separate commit

*/
#include <kernel_includes.h>
#include "../common/soc_gpio.h"
#include "../common/cypress_psoc6_dt.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of the common folder? Does it still apply to e.g. psoc5, psoc6...?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cypress uses PDL with PSoC 4 and PSoC 6. PSoC-6 is an dual core asymmetric (Cortex-M0+/M4) and PSoC-4 is a Cortex-M0+. It will be natural share some code once both shares same library.

drive-open-drain:
required: false
type: boolean
description: drive with open drain
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ST pinctrlfiles should also be adjusted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that somehow we need inform user about what props are available.

  1. Is there any consensus how to do it?
  2. I noted that slew-rate type on ST's files are string and at https://www.kernel.org/doc/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml defines as uint32:
  slew-rate:
    $ref: /schemas/types.yaml#/definitions/uint32
    description: set the slew rate

slew-rate:
required: false
type: string
default: "low-speed"
enum:
- "low-speed" # Default value.
- "medium-speed"
- "high-speed"
- "very-high-speed"
description: Pin speed. Default to low-speed. For few pins (PA11 and
PB3 depending on SoCs)hardware reset value could differ
(very-high-speed). Carefully check reference manual for these pins.

It will be better if you already have the final solution on master, so I can rebase and apply [1].

CC: @erwango , @galak

description: cypress pins
properties:
"cypress,pins":
type: phandle-array
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide a usage example of the pinctrl bindings (board)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, to have something that works need solve dependencies first: dt-util.h, IRQ and GPIO.

@nandojve nandojve changed the title soc: arm: cypress: psoc6: Introduce pinctrl [DNM] soc: arm: cypress: psoc6: Introduce pinctrl Oct 22, 2020
@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Dec 23, 2020
@github-actions github-actions bot closed this Jan 6, 2021
@nandojve nandojve deleted the atl/psoc6-zephyr-pinctrl branch February 23, 2021 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: Devicetree Binding PR modifies or adds a Device Tree binding area: Devicetree Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants