Re: [PATCH v2 1/9] ARM: dts: sam9x60: Add OHCI and EHCI DT nodes
On 04.01.2023 11:56, Marek Vasut wrote:
> On 1/4/23 08:24, eugen.hris...@microchip.com wrote:
>> On 1/3/23 18:00, Marek Vasut wrote:
>>> On 1/3/23 16:56, sergiu.m...@microchip.com wrote:
On 03.01.2023 17:47, Marek Vasut wrote:
> On 1/3/23 16:35, Sergiu Moga wrote:
>> Add the OHCI and EHCI DT nodes for the sam9x60 SoC's.
>>
>> Signed-off-by: Sergiu Moga
>> ---
>>
>> v1 -> v2:
>> - use usb@
>>
>> arch/arm/dts/sam9x60.dtsi | 18 ++
>> 1 file changed, 18 insertions(+)
>>
>> diff --git a/arch/arm/dts/sam9x60.dtsi b/arch/arm/dts/sam9x60.dtsi
>> index 17224ef771..4fcfb5c597 100644
>> --- a/arch/arm/dts/sam9x60.dtsi
>> +++ b/arch/arm/dts/sam9x60.dtsi
>> @@ -69,6 +69,24 @@
>> #size-cells = <1>;
>> ranges;
>>
>> + usb1: usb@60 {
>> + compatible = "atmel,at91rm9200-ohci",
>> "usb-ohci";
>> + reg = <0x0060 0x10>;
>> + clocks = < PMC_TYPE_PERIPHERAL 22>, <
>> PMC_TYPE_PERIPHERAL 22>, < PMC_TYPE_SYSTEM 21>;
>
> ./scripts/checkpatch.pl on this patch indicates
>
> WARNING: line length of 121 exceeds 100 columns
> #93: FILE: arch/arm/dts/sam9x60.dtsi:75:
> + clocks = < PMC_TYPE_PERIPHERAL 22>, <
> PMC_TYPE_PERIPHERAL 22>, < PMC_TYPE_SYSTEM 21>;
>
> Please run checkpatch on all your patches.
>
> Also, wait a few days before sending V2 , no need to send V2
> immediately
> while review of V1 is still ongoing.
It was my understanding that exceeding the character per line limit on
DT's is acceptable. All of our DT's are like this.
>>>
>>> Not to my knowledge or per what is in other DTs.
>>> Is that some exception here ?
>>
>> DTs have not had restrictions on 80 chars per line. Have a look in Linux.
>>
>> Also, as we have a requirement to keep the DT from Linux, we either have
>> to patch up Linux if there is a problem in the DT, or keep it in sync
>> with Linux.
>> We may have exceptions if there is a too big amount of work to be done
>> now to change things, to have differences in term of DT between Uboot
>> and Linux. But the general guideline is to keep the DT in sync with
>> Linux as it's ABI.
>
> Linux checkpatch.pl --strict does complain about the line length for
> sam9x60.dtsi , it was demoted from warning to check there.
>
> Can you send a separate follow-up patch to U-Boot checkpatch.pl which
> does the same , so we'd be in sync with Linux ?
I am sorry, I do not know at the moment where that change is supposed to
be made in checkpatch.pl. So, right now, I would rather focus my time on
updating these patch series to be good enough and only afterwards I
would take a look at where exactly I should make the change for
checkpatch.pl as well, if that's fine. Thanks for the understanding.
Re: [PATCH 3/6] dt-bindings: clk: at91: Define additional UTMI related clocks
On 03.01.2023 16:35, Marek Vasut wrote: > On 1/3/23 12:50, sergiu.m...@microchip.com wrote: >> On 03.01.2023 01:08, Marek Vasut wrote: >>> On 12/23/22 13:33, Sergiu Moga wrote: Add definitions for an additional main UTMI clock as well as its respective subclocks. Signed-off-by: Sergiu Moga --- include/dt-bindings/clk/at91.h | 5 + 1 file changed, 5 insertions(+) diff --git a/include/dt-bindings/clk/at91.h b/include/dt-bindings/clk/at91.h index e30756b280..386f01cf31 100644 --- a/include/dt-bindings/clk/at91.h +++ b/include/dt-bindings/clk/at91.h @@ -18,5 +18,10 @@ #define PMC_TYPE_PERIPHERAL 3 #define PMC_TYPE_GCK 4 #define PMC_TYPE_SLOW 5 +#define UTMI 6 + +#define UTMI1 0 +#define UTMI2 1 +#define UTMI3 2 >>> >>> Why isn't there PMC_TYPE_ prefix in these new macros ? >> >> There is no PMC_TYPE_ because it refers to a different block external to >> the PMC block. PMC feeds the UTMI clock which feeds the UTMI block that >> contains the three UTMI clocks: the one for port A and the ones meant >> for port B and C which depend on port A's UTMI clock. There is no >> control in the PMC for these. The reason why I added UTMI in this file >> is because it is related to DT clock definitions. > > Can you come up with different prefix then ? How about USB_UTMI? :) Would that be fine with you?
Re: [PATCH v2 1/3] phy: at91: Add support for the USB 2.0 PHY's of SAMA7
On 03.01.2023 17:31, Marek Vasut wrote:
> On 1/3/23 15:30, Sergiu Moga wrote:
>> In order to have USB functionality, drivers for SAMA7's
>> USB 2.0 PHY's have been added. There is one driver
>> for UTMI clock's SFR and RESET required functionalities and
>> one for its three possible subclocks of the phy's themselves.
>> In order for this layout to properly work in conjunction with
>> CCF and DT, the former driver will also act as a clock provider
>> for the three phy's with the help of a custom hook into the
>> driver's of_xlate method.
>
> [...]
>
>> +static int sama7_utmi_probe(struct udevice *dev)
>> +{
>> + struct clk *utmi_parent_clk, *utmi_clk;
>> + struct regmap *regmap_sfr;
>> + struct reset_ctl *phy_reset;
>> + int i;
>> + char name[16];
>> +
>> + utmi_parent_clk = devm_clk_get(dev, "utmi_clk");
>> + if (IS_ERR(utmi_parent_clk))
>> + return PTR_ERR(utmi_parent_clk);
>> +
>> + regmap_sfr = syscon_regmap_lookup_by_phandle(dev, "sfr-phandle");
>> + if (IS_ERR(regmap_sfr))
>> + return PTR_ERR(regmap_sfr);
>> +
>> + for (i = 0; i < ARRAY_SIZE(sama7_utmick); i++) {
>> + snprintf(name, sizeof(name), "usb%d_reset", i);
>> + phy_reset = devm_reset_control_get(dev, name);
>> + if (IS_ERR(phy_reset))
>> + return PTR_ERR(phy_reset);
>> +
>> + utmi_clk = sama7_utmi_clk_register(regmap_sfr, phy_reset,
>> + sama7_utmick[i].n,
>> + sama7_utmick[i].p,
>> + sama7_utmick[i].id);
>> + if (IS_ERR(utmi_clk))
>> + return PTR_ERR(utmi_clk);
>
> Isn't this going to leak memory if i>0 and a failure occurs ? I think it
> will, since sama7_utmi_clk_register() calls kzalloc() .
Yes, you are right. Perhaps something like this should work better
instead, wouldn't you agree?
static int sama7_utmi_probe(struct udevice *dev)
{
- struct clk *utmi_parent_clk, *utmi_clk;
+ struct clk *utmi_parent_clk, *utmi_clk[ARRAY_SIZE(sama7_utmick)];
+ struct sama7_utmi_clk *uclk;
struct regmap *regmap_sfr;
struct reset_ctl *phy_reset;
- int i;
+ int i, j;
char name[16];
utmi_parent_clk = devm_clk_get(dev, "utmi_clk");
@@ -153,12 +154,18 @@ static int sama7_utmi_probe(struct udevice *dev)
if (IS_ERR(phy_reset))
return PTR_ERR(phy_reset);
- utmi_clk = sama7_utmi_clk_register(regmap_sfr, phy_reset,
+ utmi_clk[i] = sama7_utmi_clk_register(regmap_sfr, phy_reset,
sama7_utmick[i].n,
sama7_utmick[i].p,
sama7_utmick[i].id);
- if (IS_ERR(utmi_clk))
+ if (IS_ERR(utmi_clk)) {
+ for (j = 0; j < i; j++) {
+ uclk = to_sama7_utmi_clk(utmi_clk[j]);
+ kfree(uclk);
+ }
+
return PTR_ERR(utmi_clk);
+ }
}
return 0;
Re: [PATCH v2 4/9] dt-bindings: reset: add sama7g5 definitions
On 03.01.2023 17:59, Marek Vasut wrote: > On 1/3/23 16:55, sergiu.m...@microchip.com wrote: >> On 03.01.2023 17:48, Marek Vasut wrote: >>> On 1/3/23 16:35, Sergiu Moga wrote: Upstream linux commit 5994f58977e0. Add reset bindings for SAMA7G5. At the moment only USB PHYs are included. Signed-off-by: Sergiu Moga --- v1 -> v2: - nothing include/dt-bindings/reset/sama7g5-reset.h | 10 ++ 1 file changed, 10 insertions(+) create mode 100644 include/dt-bindings/reset/sama7g5-reset.h diff --git a/include/dt-bindings/reset/sama7g5-reset.h b/include/dt-bindings/reset/sama7g5-reset.h new file mode 100644 index 00..2116f41d04 --- /dev/null +++ b/include/dt-bindings/reset/sama7g5-reset.h @@ -0,0 +1,10 @@ +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ + +#ifndef __DT_BINDINGS_RESET_SAMA7G5_H +#define __DT_BINDINGS_RESET_SAMA7G5_H + +#define SAMA7G5_RESET_USB_PHY1 4 +#define SAMA7G5_RESET_USB_PHY2 5 +#define SAMA7G5_RESET_USB_PHY3 6 >>> >>> Is there no reset with ID 0/1/2/3 ? >> >> No, there is not. > > Please just add a comment into the file then, explaining that. Would this not make the commit different from the upstream linux commit referenced in the commit message? Perhaps it would be better to place this comment in the commit message instead?
Re: [PATCH 2/4] usb: ohci-at91: Enable OHCI functionality and register into DM
On 03.01.2023 17:57, Marek Vasut wrote:
> On 1/3/23 16:49, sergiu.m...@microchip.com wrote:
>> On 03.01.2023 16:34, Marek Vasut wrote:
>>> On 1/3/23 13:11, sergiu.m...@microchip.com wrote:
On 03.01.2023 01:26, Marek Vasut wrote:
> On 12/23/22 13:34, Sergiu Moga wrote:
>
> [...]
>
>> diff --git a/drivers/usb/host/ohci-at91.c
>> b/drivers/usb/host/ohci-at91.c
>> index 9b955c1bd6..9ae55c6e5d 100644
>> --- a/drivers/usb/host/ohci-at91.c
>> +++ b/drivers/usb/host/ohci-at91.c
>> @@ -5,6 +5,9 @@
>> */
>>
>> #include
>> +
>> +#if !(CONFIG_IS_ENABLED(DM_USB))
>> +
>> #include
>>
>> int usb_cpu_init(void)
>> @@ -62,3 +65,183 @@ int usb_cpu_init_fail(void)
>> {
>> return usb_cpu_stop();
>> }
>
> Would it be possible to just remove the non-DM functionality ?
>
Unfortunately, the other older boards would not build and work anymore
if I were to remove this.
>>>
>>> Any chance they can be converted to DM ?
>>>
>>
>> Not at the moment, no.
>
> Why ?
My apologies, but I do not have these boards at my disposal to test them
and they are not the focus of this patch series. The reason I kept the
non-DM part is that so the build on the other boards does not break.
Perhaps in the future there will be plans to convert these as well but
at the moment I can not say for sure.
Re: [PATCH v2 1/9] ARM: dts: sam9x60: Add OHCI and EHCI DT nodes
On 03.01.2023 17:47, Marek Vasut wrote:
> On 1/3/23 16:35, Sergiu Moga wrote:
>> Add the OHCI and EHCI DT nodes for the sam9x60 SoC's.
>>
>> Signed-off-by: Sergiu Moga
>> ---
>>
>> v1 -> v2:
>> - use usb@
>>
>> arch/arm/dts/sam9x60.dtsi | 18 ++
>> 1 file changed, 18 insertions(+)
>>
>> diff --git a/arch/arm/dts/sam9x60.dtsi b/arch/arm/dts/sam9x60.dtsi
>> index 17224ef771..4fcfb5c597 100644
>> --- a/arch/arm/dts/sam9x60.dtsi
>> +++ b/arch/arm/dts/sam9x60.dtsi
>> @@ -69,6 +69,24 @@
>> #size-cells = <1>;
>> ranges;
>>
>> + usb1: usb@60 {
>> + compatible = "atmel,at91rm9200-ohci", "usb-ohci";
>> + reg = <0x0060 0x10>;
>> + clocks = < PMC_TYPE_PERIPHERAL 22>, <
>> PMC_TYPE_PERIPHERAL 22>, < PMC_TYPE_SYSTEM 21>;
>
> ./scripts/checkpatch.pl on this patch indicates
>
> WARNING: line length of 121 exceeds 100 columns
> #93: FILE: arch/arm/dts/sam9x60.dtsi:75:
> + clocks = < PMC_TYPE_PERIPHERAL 22>, <
> PMC_TYPE_PERIPHERAL 22>, < PMC_TYPE_SYSTEM 21>;
>
> Please run checkpatch on all your patches.
>
> Also, wait a few days before sending V2 , no need to send V2 immediately
> while review of V1 is still ongoing.
It was my understanding that exceeding the character per line limit on
DT's is acceptable. All of our DT's are like this.
Re: [PATCH v2 4/9] dt-bindings: reset: add sama7g5 definitions
On 03.01.2023 17:48, Marek Vasut wrote: > On 1/3/23 16:35, Sergiu Moga wrote: >> Upstream linux commit 5994f58977e0. >> >> Add reset bindings for SAMA7G5. At the moment only USB PHYs are >> included. >> >> Signed-off-by: Sergiu Moga >> --- >> >> v1 -> v2: >> - nothing >> >> >> include/dt-bindings/reset/sama7g5-reset.h | 10 ++ >> 1 file changed, 10 insertions(+) >> create mode 100644 include/dt-bindings/reset/sama7g5-reset.h >> >> diff --git a/include/dt-bindings/reset/sama7g5-reset.h >> b/include/dt-bindings/reset/sama7g5-reset.h >> new file mode 100644 >> index 00..2116f41d04 >> --- /dev/null >> +++ b/include/dt-bindings/reset/sama7g5-reset.h >> @@ -0,0 +1,10 @@ >> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ >> + >> +#ifndef __DT_BINDINGS_RESET_SAMA7G5_H >> +#define __DT_BINDINGS_RESET_SAMA7G5_H >> + >> +#define SAMA7G5_RESET_USB_PHY1 4 >> +#define SAMA7G5_RESET_USB_PHY2 5 >> +#define SAMA7G5_RESET_USB_PHY3 6 > > Is there no reset with ID 0/1/2/3 ? No, there is not.
Re: [PATCH 4/6] ARM: dts: at91: sama7: Add USB related DT nodes
On 03.01.2023 16:35, Marek Vasut wrote: > On 1/3/23 12:53, sergiu.m...@microchip.com wrote: >> On 03.01.2023 01:07, Marek Vasut wrote: >>> On 12/23/22 13:33, Sergiu Moga wrote: Add the USB related DT nodes for the sama7g5ek board. Signed-off-by: Sergiu Moga --- arch/arm/dts/at91-sama7g5ek.dts | 34 +++ >>> >>> Board DT and SoC DT should be separate patches. >>> >>> Are these DT changes part of upstream Linux yet ? >>> >> >> >> No, but they will be integrated in the future. > > Were the patches at least posted ? > > If not, add the nodes into at91-sama7g5ek-u-boot.dtsi , so it is clear > this is temporarily U-Boot specific. I am sorry, I somehow did not receive these replies before sending v2. Sure thing, I will try moving them there.
Re: [PATCH 2/4] usb: ohci-at91: Enable OHCI functionality and register into DM
On 03.01.2023 16:34, Marek Vasut wrote:
> On 1/3/23 13:11, sergiu.m...@microchip.com wrote:
>> On 03.01.2023 01:26, Marek Vasut wrote:
>>> On 12/23/22 13:34, Sergiu Moga wrote:
>>>
>>> [...]
>>>
diff --git a/drivers/usb/host/ohci-at91.c
b/drivers/usb/host/ohci-at91.c
index 9b955c1bd6..9ae55c6e5d 100644
--- a/drivers/usb/host/ohci-at91.c
+++ b/drivers/usb/host/ohci-at91.c
@@ -5,6 +5,9 @@
*/
#include
+
+#if !(CONFIG_IS_ENABLED(DM_USB))
+
#include
int usb_cpu_init(void)
@@ -62,3 +65,183 @@ int usb_cpu_init_fail(void)
{
return usb_cpu_stop();
}
>>>
>>> Would it be possible to just remove the non-DM functionality ?
>>>
>>
>> Unfortunately, the other older boards would not build and work anymore
>> if I were to remove this.
>
> Any chance they can be converted to DM ?
>
Not at the moment, no.
+#elif CONFIG_IS_ENABLED(DM_GPIO)
>>>
>>> I think you want plain '#else' here, and then make the driver select
>>> DM_GPIO in case DM_USB is enabled in Kconfig entry.
>>>
+#include
+#include
+#include
+#include
+#include "ohci.h"
+
+#define AT91_MAX_USBH_PORTS 3
+#define at91_for_each_port(index) \
+ for ((index) = 0; (index) < AT91_MAX_USBH_PORTS;
(index)++)
+
+struct at91_usbh_data {
+ enum usb_init_type init_type;
+ struct gpio_desc vbus_pin[AT91_MAX_USBH_PORTS];
>>>
>>> drivers/usb/host/ehci-generic.c: ret =
>>> device_get_supply_regulator(dev, "vbus-supply",
>>>
>>> I wonder if we can instead use regulator for vbus-supply here too ?
>>>
>>
>> I would rather keep the same DT properties as those in Linux and have
>> them be in concordance with our USB DT binding, which specifies the
>> atmel,vbus-gpio property and no vbus-supply.
>
> Oh well, so yes, we're stuck with this GPIO stuff.
>
> [...]
Unfortunately yes...
Re: [PATCH 4/4] usb: ohci-at91: Add USB PHY functionality
On 03.01.2023 01:33, Marek Vasut wrote:
> On 12/23/22 13:34, Sergiu Moga wrote:
>> Add the ability to enable/disable whatever USB PHY's are
>> passed to the AT91 OHCI driver through DT.
>>
>> Signed-off-by: Sergiu Moga
>> Tested-by: Mihai Sain
>> ---
>> drivers/usb/host/ohci-at91.c | 31 +++
>> 1 file changed, 31 insertions(+)
>>
>> diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
>> index 5cf8f283e5..217f31b402 100644
>> --- a/drivers/usb/host/ohci-at91.c
>> +++ b/drivers/usb/host/ohci-at91.c
>> @@ -74,6 +74,10 @@ int usb_cpu_init_fail(void)
>> #include
>> #include "ohci.h"
>>
>> +#if CONFIG_IS_ENABLED(PHY_MICROCHIP_SAMA7_USB)
>> +#include
>> +#endif
>> +
>> #define AT91_MAX_USBH_PORTS 3
>>
>> #define at91_for_each_port(index) \
>> @@ -91,6 +95,10 @@ struct ohci_at91_priv {
>> struct clk *fclk;
>> struct clk *hclk;
>> bool clocked;
>> +
>> +#if CONFIG_IS_ENABLED(PHY_MICROCHIP_SAMA7_USB)
>> + struct phy phy[AT91_MAX_USBH_PORTS];
>> +#endif
>> };
>>
>> static void at91_start_clock(struct ohci_at91_priv *ohci_at91)
>> @@ -98,6 +106,13 @@ static void at91_start_clock(struct ohci_at91_priv
>> *ohci_at91)
>> if (ohci_at91->clocked)
>> return;
>>
>> +#if CONFIG_IS_ENABLED(PHY_MICROCHIP_SAMA7_USB)
>
> Use plain:
>
> if (CONFIG_IS_ENABLED(...)) { ... }
>
> instead of the #if ... , the compiler would optimize the code out
> correctly.
>
The build system complains that the generic_phy_* methods are not
present and, if possible, I would like not to have to enable the PHY
related CONFIGs on the boards that do not need it only to avoid these
warnings.
>> + int i;
>> +
>> + at91_for_each_port(i)
>> + generic_phy_power_on(_at91->phy[i]);
>
> Look at generic_phy_get_bulk() and generic_phy_power_on_bulk() and co.
> those should get rid of this loop altogether.
>
> [...]
Re: [PATCH 2/4] usb: ohci-at91: Enable OHCI functionality and register into DM
On 03.01.2023 01:26, Marek Vasut wrote:
> On 12/23/22 13:34, Sergiu Moga wrote:
>
> [...]
>
>> diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
>> index 9b955c1bd6..9ae55c6e5d 100644
>> --- a/drivers/usb/host/ohci-at91.c
>> +++ b/drivers/usb/host/ohci-at91.c
>> @@ -5,6 +5,9 @@
>> */
>>
>> #include
>> +
>> +#if !(CONFIG_IS_ENABLED(DM_USB))
>> +
>> #include
>>
>> int usb_cpu_init(void)
>> @@ -62,3 +65,183 @@ int usb_cpu_init_fail(void)
>> {
>> return usb_cpu_stop();
>> }
>
> Would it be possible to just remove the non-DM functionality ?
>
Unfortunately, the other older boards would not build and work anymore
if I were to remove this.
>> +#elif CONFIG_IS_ENABLED(DM_GPIO)
>
> I think you want plain '#else' here, and then make the driver select
> DM_GPIO in case DM_USB is enabled in Kconfig entry.
>
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include "ohci.h"
>> +
>> +#define AT91_MAX_USBH_PORTS 3
>>
>> +#define at91_for_each_port(index) \
>> + for ((index) = 0; (index) < AT91_MAX_USBH_PORTS; (index)++)
>> +
>> +struct at91_usbh_data {
>> + enum usb_init_type init_type;
>> + struct gpio_desc vbus_pin[AT91_MAX_USBH_PORTS];
>
> drivers/usb/host/ehci-generic.c: ret =
> device_get_supply_regulator(dev, "vbus-supply",
>
> I wonder if we can instead use regulator for vbus-supply here too ?
>
I would rather keep the same DT properties as those in Linux and have
them be in concordance with our USB DT binding, which specifies the
atmel,vbus-gpio property and no vbus-supply.
Other than that, I am fine with the other requested changes.
>> + u8 ports; /* number of ports on
>> root hub */
>> +};
>> +
>> +struct ohci_at91_priv {
>> + struct clk *iclk;
>> + struct clk *fclk;
>> + struct clk *hclk;
>
> Have a look at clk.*bulk functions in U-Boot , then you can do
> clk_get_bulk() / clk_enable_bulk() etc. below.
>
>> + bool clocked;
>> +};
>> +
>> +static void at91_start_clock(struct ohci_at91_priv *ohci_at91)
>> +{
>> + if (ohci_at91->clocked)
>> + return;
>> +
>> + clk_set_rate(ohci_at91->fclk, 4800);
>> + clk_prepare_enable(ohci_at91->hclk);
>> + clk_prepare_enable(ohci_at91->iclk);
>> + clk_prepare_enable(ohci_at91->fclk);
>
> E.g. here you can use clk_enable_bulk() .
>
>> + ohci_at91->clocked = true;
>
> Why is this here, are you suffering from clock inbalance ? You
> shouldn't, so remove this and the if (ohci_at91->clocked) test above.
>
>> +}
>> +
>> +static void at91_stop_clock(struct ohci_at91_priv *ohci_at91)
>> +{
>> + if (!ohci_at91->clocked)
>
> Is this really needed ?
>
>> + return;
>> +
>> + clk_disable_unprepare(ohci_at91->fclk);
>> + clk_disable_unprepare(ohci_at91->iclk);
>> + clk_disable_unprepare(ohci_at91->hclk);
>> + ohci_at91->clocked = false;
>> +}
>> +
>> +static void ohci_at91_set_power(struct at91_usbh_data *pdata, int port,
>> + bool enable)
>> +{
>> + if (!dm_gpio_is_valid(>vbus_pin[port]))
>> + return;
>
> This could be regulator instead.
>
>> + if (enable)
>> + dm_gpio_set_dir_flags(>vbus_pin[port],
>> + GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);
>> + else
>> + dm_gpio_set_dir_flags(>vbus_pin[port], 0);
>> +}
>> +
>> +static void at91_start_hc(struct udevice *dev)
>> +{
>> + struct ohci_at91_priv *ohci_at91 = dev_get_priv(dev);
>> +
>> + at91_start_clock(ohci_at91);
>> +}
>> +
>> +static void at91_stop_hc(struct udevice *dev)
>> +{
>> + struct ohci_at91_priv *ohci_at91 = dev_get_priv(dev);
>> +
>> + at91_stop_clock(ohci_at91);
>> +}
>> +
>> +static int ohci_atmel_deregister(struct udevice *dev)
>> +{
>> + struct at91_usbh_data *pdata = dev_get_plat(dev);
>> + int i;
>> +
>> + at91_stop_hc(dev);
>> +
>> + at91_for_each_port(i) {
>> + if (i >= pdata->ports)
>
> You can just wrap this test into the at91_for_each_port() macro , just
> use "index < min(AT91_MAX_USBH_PORTS, pdata->ports)" or so.
>
>> + break;
>> +
>> + ohci_at91_set_power(pdata, i, false);
>> + }
>> +
>> + return ohci_deregister(dev);
>> +}
>> +
>> +static int ohci_atmel_child_pre_probe(struct udevice *dev)
>> +{
>> + struct udevice *ohci_controller = dev_get_parent(dev);
>> + struct at91_usbh_data *pdata = dev_get_plat(ohci_controller);
>> + int i;
>> +
>> + at91_for_each_port(i) {
>> + if (i >= pdata->ports)
>> + break;
>> +
>> + ohci_at91_set_power(pdata, i, true);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int ohci_atmel_probe(struct udevice *dev)
>> +{
>> + struct at91_usbh_data *pdata = dev_get_plat(dev);
>> + struct ohci_at91_priv *ohci_at91 = dev_get_priv(dev);
>> + int i;
>> + int
Re: [PATCH 4/6] ARM: dts: at91: sama7: Add USB related DT nodes
On 03.01.2023 01:07, Marek Vasut wrote: > On 12/23/22 13:33, Sergiu Moga wrote: >> Add the USB related DT nodes for the sama7g5ek board. >> >> Signed-off-by: Sergiu Moga >> --- >> arch/arm/dts/at91-sama7g5ek.dts | 34 +++ > > Board DT and SoC DT should be separate patches. > > Are these DT changes part of upstream Linux yet ? > No, but they will be integrated in the future. >> arch/arm/dts/sama7g5.dtsi | 73 + >> 2 files changed, 107 insertions(+) > > [...] >
Re: [PATCH 3/6] dt-bindings: clk: at91: Define additional UTMI related clocks
On 03.01.2023 01:08, Marek Vasut wrote: > On 12/23/22 13:33, Sergiu Moga wrote: >> Add definitions for an additional main UTMI clock as well as its >> respective subclocks. >> >> Signed-off-by: Sergiu Moga >> --- >> include/dt-bindings/clk/at91.h | 5 + >> 1 file changed, 5 insertions(+) >> >> diff --git a/include/dt-bindings/clk/at91.h >> b/include/dt-bindings/clk/at91.h >> index e30756b280..386f01cf31 100644 >> --- a/include/dt-bindings/clk/at91.h >> +++ b/include/dt-bindings/clk/at91.h >> @@ -18,5 +18,10 @@ >> #define PMC_TYPE_PERIPHERAL 3 >> #define PMC_TYPE_GCK 4 >> #define PMC_TYPE_SLOW 5 >> +#define UTMI 6 >> + >> +#define UTMI1 0 >> +#define UTMI2 1 >> +#define UTMI3 2 > > Why isn't there PMC_TYPE_ prefix in these new macros ? There is no PMC_TYPE_ because it refers to a different block external to the PMC block. PMC feeds the UTMI clock which feeds the UTMI block that contains the three UTMI clocks: the one for port A and the ones meant for port B and C which depend on port A's UTMI clock. There is no control in the PMC for these. The reason why I added UTMI in this file is because it is related to DT clock definitions.
Re: [PATCH 0/4] Register at91 OHCI into DM and add SAMA7 USB PHY's
On 23.12.2022 14:34, Sergiu Moga wrote: > This patch series originates from a bigger patch series: > https://lists.denx.de/pipermail/u-boot/2022-December/502865.html > > Register ohci-at91 driver into Driver Model. In order for the VBUS to > stay enabled, a `child_pre_probe` method has been added to overcome the > DM core disabling it in `usb_scan_device`: when the generic `device_probe` > method is called, the pinctrl is processed once again, undoing whatever > changes have been made in our driver's probe method. > In order to enable USB on SAMA7G5 the addition of the USB 2.0 PHY > drivers were required. > > Cristian Birsan (1): >usb: ohci-at91: Add `ohci_t` field in `ohci_at91_priv` > > Sergiu Moga (3): >phy: at91: Add support for the USB 2.0 PHY's of SAMA7 >usb: ohci-at91: Enable OHCI functionality and register into DM >usb: ohci-at91: Add USB PHY functionality > > drivers/phy/Kconfig | 10 ++ > drivers/phy/Makefile | 1 + > drivers/phy/phy-sama7-usb.c | 90 + > drivers/phy/phy-sama7-utmi-clk.c | 202 + > drivers/usb/host/ohci-at91.c | 215 +++ > 5 files changed, 518 insertions(+) > create mode 100644 drivers/phy/phy-sama7-usb.c > create mode 100644 drivers/phy/phy-sama7-utmi-clk.c > By the way, this also depends on the following patch series: https://lists.denx.de/pipermail/u-boot/2022-December/502979.html
Re: [PATCH v5 00/19] Add USB on SAM9X60, SAMA7G5 and SAMA5D2 boards
On 22.12.2022 15:36, Marek Vasut wrote: > On 12/22/22 11:53, Sergiu Moga wrote: >> This series of patches is meant to add support for USB Mass Storage >> on SAM9X60, SAMA7G5 and SAMA5D2 boards and register ohci-at91 driver into >> Driver Model. In order for this to be achieved, the respective >> DT nodes have been added, the USB clock has been registered into CCF >> and the required defconfigs have been added to the boards' defconfig. >> What is more, in order for the VBUS to stay enabled, a `child_pre_probe` >> method has been added to overcome the DM core disabling it in >> `usb_scan_device`: when the generic `device_probe` method is called, >> the pinctrl is processed once again, undoing whatever changes have >> been made in our driver's probe method. >> In order to enable USB on SAMA7G5 the addition of RSTC and USB 2.0 PHY >> drivers were required. > > Please split the series into more manageable parts -- architecture and > DT bits, board, clock, usb . Hi, I am sorry but I believe that they are ordered correctly and they all should be part of this series as it ensures usb across all boards and most of them depend on each other. I think the order in which they have been sent makes the most sense: adding what is needed by sam9x60, followed by sama7g5, sama5d2 and finally enabling them all in the defconfigs. I am not sure that sending multiple series that are dependent on each other would make it easier.
